sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.43k stars 479 forks source link

Doctest failures caused by non-working sympow on 32-bit Solaris x86 and 32-bit OpenSolaris #9703

Closed bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 closed 14 years ago

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago

John Palmieri has built most of Sage on the host fulvia, but there are a number of tests related to sympow that are failing. The summary at the end shows:

The following tests failed:
sage -t  -long devel/sage/sage/modular/hecke/submodule.py # 1 doctests failed
sage -t  -long devel/sage/sage/modular/abvar/abvar.py # 1 doctests failed
sage -t  -long devel/sage/sage/lfunctions/sympow.py # 13 doctests failed
sage -t  -long devel/sage/sage/stats/hmm/chmm.pyx # 3 doctests failed
sage -t  -long devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py # 17 doctests failed
sage -t  -long devel/sage/sage/symbolic/expression.pyx # 2 doctests failed
Total time for all tests: 7305.2 seconds

of these test failures, the following three are attributed to SYMPOW

sage -t  -long devel/sage/sage/modular/abvar/abvar.py # 1 doctests failed
sage -t  -long devel/sage/sage/lfunctions/sympow.py # 13 doctests failed
sage -t  -long devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py # 17 doctests failed

Looking at the source code, it is not valid C, so it's quite possible the code gets mis-compiled. In fact, IMHO, gcc should reject the code - just as the Sun compiler does.

I'll try to work out what was intended and see if the code can be re-written in a way that compiles with the Sun compiler, in which case gcc should have more chance of generating correct code.

Upstream: Reported upstream. Little or no feedback.

CC: @jhpalmieri @jaapspies @nexttime @peterjeremy @kiwifb @williamstein @JohnCremona @mwhansen

Component: build

Author: David Kirkby

Reviewer: Jeroen Demeyer, Mitesh Patel, Carl Witty

Merged: sage-4.5.3.alpha2

Issue created by migration from https://trac.sagemath.org/ticket/9703

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago
comment:1

The same issue is observed on a 32-bit build on OpenSolaris.

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,4 @@
 John Palmieri has built most of Sage on the host fulvia, but there are a number of tests related to sympow that are failing. The summary at the end shows:
-

The following tests failed:

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago
comment:3

By generating the data files directly, with SAGE_LOCAL/lib/sympow/new_data, I was able to get the "optional" and "not tested" examples here to run. To really run ./sympow -new_data 2 for the example a = sympow.L(EllipticCurve('11a'), 2, 16); a, I ran

$ ./sage -sh
> cd SAGE_LOCAL/lib/sympow
> ./new_data sh gp '-sp 2'

For the example print sympow.Lderivs(EllipticCurve('11a'), 1, 16, 2), I ran

$ ./sage -sh
> cd SAGE_LOCAL/lib/sympow
> ./new_data sh gp '-sp 1 -dv 0'
> ./new_data sh gp '-sp 1 -dv 1'
> ./new_data sh gp '-sp 1 -dv 2'

It seems that part of the Sage interface to SYMPOW is broken.

But I don't know if missing data files caused the failures reported here and at #9166.

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago
comment:4

Replying to @qed777:

It seems that part of the Sage interface to SYMPOW is broken.

Or maybe sympow itself doesn't properly call new_data?

jhpalmieri commented 14 years ago
comment:5

On the other hand, there is this (after running "sage -sh" and "cd $SAGE_LOCAL/lib/sympow"):

$ ./sympow -curve "[0,-1,1,-10,-20]" -moddeg
sympow 1.018 RELEASE  (c) Mark Watkins -**ERROR** QD_check failed at x[1]

On my mac or on sage.math, this produces valid output, including the line

Modular Degree is 1

which is what is parsed by the modular_degree method. So it looks to me as though sympow is really broken, not just its interface.

(Furthermore, regardless of the C code, I think that sympow.py would never be accepted into Sage today: too many untested methods.)

jhpalmieri commented 14 years ago
comment:6

I wasn't clear enough above: the line **ERROR**... was produced on fulvia, an x86 box running Solaris.

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago
comment:7

Replying to @qed777:

By generating the data files directly, with SAGE_LOCAL/lib/sympow/new_data, I was able to get the "optional" and "not tested" examples here to run. To really run ./sympow -new_data 2 for the example

Try that on the global installation of sage on sage.math - not your own private one.

I'm told part of the problem is that SYMPOW is trying to write data files below its installation directory. So it fails on a global installation of Sage, unless the user the user has root access.

But there's very little error checking, so the fact the data files are not created is not reported.

It seems that part of the Sage interface to SYMPOW is broken.

If it was only that!

But I don't know if missing data files caused the failures reported here and at #9166.

To the best of my knowledge, not all examples need data files. When they do, a message is printed telling one how to create the data files. So that is not all the reason. That is certainly applied by the README file in the source code.

I've run the smypow executable from the command line, and find problems unrelated to the generation of data files.

It's not just the C code that is bad. There's a Configure script that starts with #!/bin/sh, then has code which checks if sh is on your system or not!

My preference would be to remove this, or at least move it to experimental, but William seems keen to keep it as a standard package. It seems a bit strange to me given.

That seems an alful lot of problems

I'm cc'ing a few people that I know have looked at SYMPOW at some point in the past. They might have some comments. Perhaps William has some comments to he would like to add to the ticket.

Dave

kiwifb commented 14 years ago
comment:8

Replying to @sagetrac-drkirkby:

Try that on the global installation of sage on sage.math - not your own private one.

I'm told part of the problem is that SYMPOW is trying to write data files below its installation directory. So it fails on a global installation of Sage, unless the user the user has root access.

I'll comment. I am the one who told that to David. I have shamelessly sat on this info for about 3 years. I noticed it when I first tried to package sympow for Gentoo as part of my sage porting drive. At the time I had to allow the directory where the sympow scripts are installed to be world writable....

That no one really noticed means that no one using a global sage install has been using sympow for their work. The only people using it must have used a private installation. We now have a fix for this problem in sage-on-gentoo, Christopher, the other sage-on-gentoo dev, has figured a way of getting sympow to write in $HOME/.sympow in the last month or so.

I think there is a case for trying to replace sympow. It seems like the original author has abandoned it - the sympow homepage has gone. Sage could become sympow upstream. But given the quality of the code there is a high burden for maintenance. It probably would be more beneficial in the long run to rewrite the functionality altogether. Given that there are references I think it could be a nice student project.

That's my opinion.

Francois

ba2de90a-bb52-4931-8573-7586330e60db commented 14 years ago
comment:9

Replying to @kiwifb:

That no one really noticed means that no one using a global sage install has been using sympow for their work. The only people using it must have used a private installation. We now have a fix for this problem in sage-on-gentoo, Christopher, the other sage-on-gentoo dev, has figured a way of getting sympow to write in $HOME/.sympow in the last month or so.

I prepared a small patch which should fix the directory/permission problems. I did not fully test it because I got little time but you should be able see its basic idea - it includes the sed fixes from our sage-on-gentoo overlay: http://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/sympow/sympow-1.018-r1.ebuild (look at it for more detailed comments on what the patch does). To successfully use it, apply it to sympow's source and do the following (would go into spkg-install):

With the patch you also should be able to create the precomputed data which is needed for some optional tests.

Christopher

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago

Description changed:

--- 
+++ 
@@ -2,12 +2,23 @@

The following tests failed: -

+of these test failues, the following three are attributed to SYMPOW + +```

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago

Upstream: Reported upstream. Little or no feedback.

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago

Author: David Kirkby

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago
comment:11

I now have a better understanding of why SYMPOW is failing to work correctly on Cygwin and Solaris on x86 chips. SYMPOW makes use of the Quad Double code which relies on IEE-754 double-precision maths, with a 53-bit mantissa and 64-bits in total. However, the default mode of the x86 CPUs is to use extended precision where 64-bits are used for the mantissa and 80 bits in total. It is necessary to change the mode of operation of the floating point processor in order that the Quad double code will work at all. SYMPOW had code for this in a file fpu.c, but it only worked on Linux. It made use of Linux-specific header files too.

The main change necessary was to add some inline assembly code to alter the values of bits 8 and 9 in the 16-bit control register of the floating point processor. I got the relevant information about the exact bits needed from an old (1987) copy of the Intel 80387 Programmer's Reference Manual.

Here's a revised package which builds and passes all relevant tests on

It was not possible to test on Solaris 10 x86, as access to the Skynet machines is currently unavailable, but I do not believe ll present any problems, since it builds fine on OpenSolaris as a 32-bit application.

http://boxen.math.washington.edu/home/kirkby/patches/sympow-1.018.1.p8.spkg

Apart from a couple of minor changes, it is the same code which Mike Hansen tested on Cygwin and said it works there too.

I've not implemented Christopher's changes which are necessary to get this to pass all the optional tests. Someone who is more keen than me can open a ticket for that if they wish. I just want to see the back of this rather annoying package.

A note to reviewers (mainly aimed at Leif!) I know there are 1001 things wrong with this package, but I have no desire to clean them all up. Please review on the basis of the actual changes I made - not what it might be desirable to make. Otherwise it will never get reviewed.

Test results a Sun Ultra 27 running OpenSolaris 06/2009 (Originally SYMPOW failed all tests on this workstation)

drkirkby@hawk:~/32/sage-4.5.3.alpha1$ uname -a
SunOS hawk 5.11 snv_134 i86pc i386 i86pc
drkirkby@hawk:~/32/sage-4.5.3.alpha1$ cat /etc/release
                       OpenSolaris Development snv_134 X86
           Copyright 2010 Sun Microsystems, Inc.  All Rights Reserved.
                        Use is subject to license terms.
                             Assembled 01 March 2010
drkirkby@hawk:~/32/sage-4.5.3.alpha1$ ./sage -t  -long devel/sage/sage/modular/abvar/abvar.py
sage -t -long "devel/sage/sage/modular/abvar/abvar.py"      
     [27.4 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 27.5 seconds
drkirkby@hawk:~/32/sage-4.5.3.alpha1$ ./sage -t  -long devel/sage/sage/lfunctions/sympow.py
sage -t -long "devel/sage/sage/lfunctions/sympow.py"        
     [5.8 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 5.8 seconds
drkirkby@hawk:~/32/sage-4.5.3.alpha1$ ./sage -t  -long devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py
sage -t -long "devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py"
     [212.8 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 212.8 seconds
drkirkby@hawk:~/32/sage-4.5.3.alpha1$ 

Test results on Ubunta Linux (sage.math)

kirkby@sage:~/sage-4.5.alpha4$ uname -a
Linux sage.math.washington.edu 2.6.24-28-server #1 SMP Sat Jul 31 18:41:35 UTC 2010 x86_64 GNU/Linux
kirkby@sage:~/sage-4.5.alpha4$ cat /etc/issue
Ubuntu 8.04.4 LTS \n \l

kirkby@sage:~/sage-4.5.alpha4$ ./sage -t  -long devel/sage/sage/modular/abvar/abvar.py
sage -t -long "devel/sage/sage/modular/abvar/abvar.py"      
     [34.1 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 34.1 seconds
kirkby@sage:~/sage-4.5.alpha4$ ./sage -t  -long devel/sage/sage/lfunctions/sympow.py
sage -t -long "devel/sage/sage/lfunctions/sympow.py"        
     [9.1 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 9.1 seconds
kirkby@sage:~/sage-4.5.alpha4$ ./sage -t  -long devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py
sage -t -long "devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py"
     [170.5 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 170.6 seconds

== Test results on OS X (bsd.math) ==

[kirkby@bsd sage-4.5.rc1]$ uname -a
Darwin bsd.local 10.4.0 Darwin Kernel Version 10.4.0: Fri Apr 23 18:28:53 PDT 2010; root:xnu-1504.7.4~1/RELEASE_I386 i386 i386 MacPro1,1 Darwin
[kirkby@bsd sage-4.5.rc1]$ ./sage -t  -long devel/sage/sage/modular/abvar/abvar.py
sage -t -long "devel/sage/sage/modular/abvar/abvar.py"      
     [40.5 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 40.5 seconds
[kirkby@bsd sage-4.5.rc1]$ ./sage -t  -long devel/sage/sage/lfunctions/sympow.py
sage -t -long "devel/sage/sage/lfunctions/sympow.py"        
     [7.6 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 7.7 seconds
[kirkby@bsd sage-4.5.rc1]$ ./sage -t  -long devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py
sage -t -long "devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py"
     [184.0 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 184.0 seconds

== Test results on a Sun T5240 SPARC running Solaris 10 update 7 (t2.math) ==

kirkby@t2:32 ~/t2/32/sage-4.5.3.alpha0$ uname -a
SunOS t2 5.10 Generic_141414-02 sun4v sparc SUNW,T5240
kirkby@t2:32 ~/t2/32/sage-4.5.3.alpha0$ cat /etc/release
                       Solaris 10 5/09 s10s_u7wos_08 SPARC
           Copyright 2009 Sun Microsystems, Inc.  All Rights Reserved.
                        Use is subject to license terms.
                             Assembled 30 March 2009
kirkby@t2:32 ~/t2/32/sage-4.5.3.alpha0$ 
kirkby@t2:32 ~/t2/32/sage-4.5.3.alpha0$ ./sage -t  -long devel/sage/sage/modular/abvar/abvar.py
sage -t -long "devel/sage/sage/modular/abvar/abvar.py"      
         [321.2 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 321.5 seconds
kirkby@t2:32 ~/t2/32/sage-4.5.3.alpha0$ ./sage -t  -long devel/sage/sage/lfunctions/sympow.py
sage -t -long "devel/sage/sage/lfunctions/sympow.py"        
         [89.0 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 89.2 seconds
kirkby@t2:32 ~/t2/32/sage-4.5.3.alpha0$ ./sage -t  -long devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py
sage -t -long "devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py"
         [3219.6 s]

----------------------------------------------------------------------
All tests passed!
Total time for all tests: 3219.8 seconds

Dave

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago

Changes the FPU to double-precession IEEE-754 from Intel's expended precisiion.

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago
comment:12

Attachment: 9703-SYMPOW-fix-for-Solaris.patch.gz

If #9722 is ready to merge soon and this ticket is also positively reviewed, I'll consider including this in a 4.5.3.alpha2.

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago
comment:13

Replying to @qed777:

If #9722 is ready to merge soon and this ticket is also positively reviewed, I'll consider including this in a 4.5.3.alpha2.

And #9693, too.

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago
comment:14

Replying to @qed777:

Replying to @qed777:

If #9722 is ready to merge soon and this ticket is also positively reviewed, I'll consider including this in a 4.5.3.alpha2.

And #9693, too.

Thank you. That would be great. That should give all doc tests passing on Solaris 10 x86, OpenSolaris and Solaris 10 SPARC.

I just need someone to change it to positive. I think I've provided quite a bit of evidence that it passes the tests on every platform I can test on.

jdemeyer commented 14 years ago

Attachment: testfpu.c.gz

Test whether FPU is 53-bit or more

jdemeyer commented 14 years ago
comment:15

Maybe the attached program might be useful. It runs a simple computation to check whether or not the FPU is 53-bit or more. For example, on a x86_64 Intel machine, I get:

gcc -mfpmath=387 testfpu.c -o testfpu && ./testfpu 134217728 134217728
a = 134217728, b = 134217728
result = 1
gcc -mfpmath=sse testfpu.c -o testfpu && ./testfpu 134217728 134217728
a = 134217728, b = 134217728
result = 0
gcc -mfpmath=387 -mpc64 testfpu.c -o testfpu && ./testfpu 134217728 134217728
a = 134217728, b = 134217728
result = 0

You could use to check

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 14 years ago
comment:16

Sage 4.6.prealpha3, which includes this ticket, passed all tests (ptestlong) on 64-bit Fedora 13 (Core2, gcc 4.4.4), see also #9343 comment:308 .

jdemeyer commented 14 years ago
comment:17

Sage 4.6.prealpha3, which includes this ticket, passed all tests (ptestlong) on a Sun UltraSPARC with Solaris 10.

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago
comment:18

Replying to @jdemeyer:

Maybe the attached program might be useful. It runs a simple computation to check whether or not the FPU is 53-bit or more. For example, on a x86_64 Intel machine, I get:

It is an interesting little program. It does show the behavior someone (probably you) suspected, that in 64-bit builds, the behaviour changes.

You could use to check

  • whether or not the patch needs to be applied
  • whether the result is correct after possibly applying the patch (do this on all systems!)

I don't personally feel there is a need to do this.

I think if the result is incorrect (i.e. for some reason the FPU remained in the wrong precision), the doctests will pick this up very quickly. This was the reason the program built, but failed tests on Cygwin and Solairs x86.

Dave

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago
comment:19

Replying to @nexttime:

Sage 4.6.prealpha3, which includes this ticket, passed all tests (ptestlong) on 64-bit Fedora 13 (Core2, gcc 4.4.4), see also #9343 comment:308 .

Thank you for the Fedora confirmation. I had already tested on Ubunta, but not Fedora.

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago
comment:20

Replying to @jdemeyer:

Sage 4.6.prealpha3, which includes this ticket, passed all tests (ptestlong) on a Sun UltraSPARC with Solaris 10.

Note the SPARC that Jeroen used (a Sun Blade 1000 belonging to me), was not the same as as I used, which was the Sun T5240 (t2.,math).

So that's confirmation on two different Sun systems. One running the 2005 release of Solaris 10, the other running a much more recent release.

Dave

jdemeyer commented 14 years ago
comment:21

Replying to @sagetrac-drkirkby:

You could use to check

  • whether or not the patch needs to be applied
  • whether the result is correct after possibly applying the patch (do this on all systems!)

I don't personally feel there is a need to do this.

  • It makes use of gcc-specific options.

It doesn't. I just added those options to show how the behaviour changes. My idea would be to run testfpu.c without any options and check the result.

  • It was not done before, so I end up making further changes to a package which is already a complete mess.

I believe that my program is a cleaner solution. For example, are you sure you don't need the patch on Darwin? Also, the following code looks very suspicious:

for machine_hardware in ix86 i386 i486 i586 i686 x86_64 i86pc ia64
jdemeyer commented 14 years ago
comment:22

By the way, my comment should not prevent this patch from being included in 4.5.3 if that was the plan. If this patch works, I'm certainly happy with that. I just wanted to indicate that there might be better ways to solve the issue.

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago
comment:23

Replying to @jdemeyer:

By the way, my comment should not prevent this patch from being included in 4.5.3 if that was the plan. If this patch works, I'm certainly happy with that. I just wanted to indicate that there might be better ways to solve the issue.

The:

for machine_hardware in ix86 i386 i486 i586 i686 x86_64 i86pc ia64

was there before - I only added i86pc and changed x to machine_hardware, as I thought it was a bit more informative.

The fpu patch has never been applied on OS X - note the original code specifically checked the platform was Linux. But since the C code used Linux-specific header files, I could not simply apply it everywhere.

Given SYMPOW is going to be withdrawn from Sage since it's poorly written and unmaintained, I think this is enough. You may notice today that Bill said on sage-devel that the author has made it clear he has no desire to maintain this package, as it works on his computer.

I really want to avoid adding things to this code, since no matter what I do, it is still going to be very poor. This solves the only remaining Solaris x86 problem and is one less problem for the Cygwin port.

Dave

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago

Reviewer: Jeroen Demeyer, Mitesh Patel

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago
comment:24

I'm testing

http://boxen.math.washington.edu/home/kirkby/patches/sympow-1.018.1.p8.spkg

on bsd, redhawk, sage, and t2.math.

The patch looks good to me, except that I'm not qualified to review the new fpu.c. But apart from that, I think we have a positive review.

Jeroen, can you comment on fpu.c? Alternatively, David, could you find someone to review this file?

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago
comment:26

Can we close #9166 if/when this ticket is merged?

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago
comment:27

Replying to @qed777:

Can we close #9166 if/when this ticket is merged?

It should be possible, but you should wait for confirmation from Mike Hansen.

Mike confirmed the earlier version I wrote had worked on Cygwin and passed all tests. However, I had did make a small change since then.

The change was only the way I wrote the numbers in fpu.c. Initially I wrote them as binary using 0b in front of the 0's and 1's. Later I converted the two binary numbers to hex, so used 0x}} instead. I found using {{{0b is not portable - it works with gcc, but not Sun Studio.

So the only changes from the patch that Mike said worked were

Dave

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago
comment:28

Oops, I should have previewed that once more.

0b was changed to 0x and the numbers obviously changed from a string of 0's and 1's to the hexadecimal equivalents.

bac7d3ea-3f1b-4826-8464-f0b53d5e12d2 commented 14 years ago
comment:29

Replying to @qed777:

I'm testing

http://boxen.math.washington.edu/home/kirkby/patches/sympow-1.018.1.p8.spkg

on bsd, redhawk, sage, and t2.math.

The patch looks good to me, except that I'm not qualified to review the new fpu.c. But apart from that, I think we have a positive review.

Jeroen, can you comment on fpu.c? Alternatively, David, could you find someone to review this file?

I've asked on sage-devel if someone can comment on it. I suspect Jeroen can. Here's the

original fpu.c

and here's the changed version

patched fpu.c

One obvious difference is the original code just writes a number to the control word, but I first read the control word, set two bits, then write the control word.

The reason I did that was that one of the bits in that control word is reserved by Intel, so I did not think it wise to write a number to the register directly.

Having carefully read the documentation that comes with the later version of quad-double, the only changes that should be made to that code is the precision control bits, which is all I change.

Dave

ba94b9bb-195b-4422-a5e2-176920eaa163 commented 14 years ago
comment:30

I've read and understood the new and old versions of fpu.c, and I confirm that the new version is equivalent to the old version under Linux but is more portable in general. So positive review for fpu.c .

(I'll go ahead and mark as positive review, relying on mpatel's review for everything except fpu.c .)

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago
comment:31

Thanks, Carl!

Replying to @qed777:

I'm testing

http://boxen.math.washington.edu/home/kirkby/patches/sympow-1.018.1.p8.spkg

on bsd, redhawk, sage, and t2.math.

The long doctests pass on these machines.

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago

Changed reviewer from Jeroen Demeyer, Mitesh Patel to Jeroen Demeyer, Mitesh Patel, Carl Witty

e14f4152-4982-4ace-8c95-73a0599b109b commented 14 years ago

Merged: sage-4.5.3.alpha2

fchapoton commented 9 years ago

Description changed:

--- 
+++ 
@@ -2,22 +2,21 @@

The following tests failed:

-of these test failues, the following three are attributed to SYMPOW +of these test failures, the following three are attributed to SYMPOW

-   sage -t  -long devel/sage/sage/modular/abvar/abvar.py # 1 doctests failed
-   sage -t  -long devel/sage/sage/lfunctions/sympow.py # 13 doctests failed
-   sage -t  -long devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py # 17 doctests failed
-
+sage -t  -long devel/sage/sage/modular/abvar/abvar.py # 1 doctests failed
+sage -t  -long devel/sage/sage/lfunctions/sympow.py # 13 doctests failed
+sage -t  -long devel/sage/sage/schemes/elliptic_curves/ell_rational_field.py # 17 doctests failed

Looking at the source code, it is not valid C, so it's quite possible the code gets mis-compiled. In fact, IMHO, gcc should reject the code - just as the Sun compiler does.