Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#10252 closed defect (duplicate)

ecm does not compile on some 32-bit Linux systems

Reported by: jdemeyer Owned by: tbd
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: packages: standard Keywords: ecm spkg-install SSE2
Cc: zimmerma, dimpase, leif, mhansen Merged in:
Authors: Reviewers: Leif Leonhardy
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: Commit:
Dependencies: #5847 Stopgaps:

Description (last modified by leif)

On the Skynet machine cicero and on a 32-bit Gentoo Linux Pentium D machine of my own, the ecm package from #5847 does not install in Sage.

The problem is with Sage's spkg-install as the pure upstream sources in src/ work fine.

The following works:

$ ./configure
$ make

The following (emulating spkg-install) does NOT work:

$ export CFLAGS="-g -O3 -fPIC"
$ ./configure
$ make

It fails with the error

spv.c: In function 'spv_pwmul':
spv.c:157: error: unknown register name '%xmm7' in 'asm'
spv.c:157: error: unknown register name '%xmm6' in 'asm'
spv.c:157: error: unknown register name '%xmm5' in 'asm'
spv.c:157: error: unknown register name '%xmm3' in 'asm'
spv.c:157: error: unknown register name '%xmm2' in 'asm'
spv.c:157: error: unknown register name '%xmm1' in 'asm'
spv.c:157: error: unknown register name '%xmm0' in 'asm'

It doesn't actually matter what CFLAGS is set to, it fails even with

$ export CFLAGS=" "

This is probably because Sage's CFLAGS overrides ecm's CFLAGS. I think the easiest solution is simply not set any CFLAGS in ecm's spkg-install. Since ecm really wants to optimize for a particular machine, I think the fact that ecm's CFLAGS are not used has also bad consequences for speed.


Actually an upstream bug... (And not setting CFLAGS or CC is not an option. There are different ways to let the compiler produce processor-specific optimized code. Note that this anyway doesn't affect optimized assembly code, which ECM also comes with.)

New spkg (ecm-6.3.p2) fixing this (including the upstream patch) at #5847.


This ticket can be closed when #5847 got merged.

Change History (34)

comment:1 Changed 7 years ago by jdemeyer

  • Cc zimmerma dimpase leif mhansen added

comment:2 follow-up: Changed 7 years ago by zimmerma

note that GMP-ECM uses CC and CFLAGS from GMP, which ensures both portability and performance. Thus overriding the choice of CFLAGS by GMP-ECM is a bad idea.

What does grep CFLAGS gmp.h give on that machine?

Paul

comment:3 in reply to: ↑ 2 ; follow-up: Changed 7 years ago by jdemeyer

Replying to zimmerma:

What does grep CFLAGS gmp.h give on that machine?

On my machine (32-bit Gentoo Linux Pentium D), I get with the mpir header installed by Sage:

#define __GMP_CFLAGS "-g -O3 "
#define __MPIR_CFLAGS "-g -O3 "

With the system-wide /usr/include/gmp.h, I get

#define __GMP_CFLAGS "-pipe -march=nocona -O2 -mfpmath=sse"

comment:4 in reply to: ↑ 3 ; follow-up: Changed 7 years ago by zimmerma

Replying to jdemeyer:

Replying to zimmerma:

What does grep CFLAGS gmp.h give on that machine?

On my machine (32-bit Gentoo Linux Pentium D), I get with the mpir header installed by Sage:

#define __GMP_CFLAGS "-g -O3 "
#define __MPIR_CFLAGS "-g -O3 "

With the system-wide /usr/include/gmp.h, I get

#define __GMP_CFLAGS "-pipe -march=nocona -O2 -mfpmath=sse"

the spkg should work with the system-wide CFLAGS value. With -g -O3 it seems HAVE_SSE2 is defined but SSE2 is not supported.

What is strange is that the GMP-ECM configure checks if SSE2 is really supported, and if it is not the SSE2 code should not be compiled.

Do you confirm that the upstream package works with CFLAGS=-g -O3?

Paul

comment:5 in reply to: ↑ 4 Changed 7 years ago by jdemeyer

Replying to zimmerma:

Do you confirm that the upstream package works with CFLAGS=-g -O3?

No, the upstream package does not work with those CFLAGS. The reports in this bug report concerned the upstream sources (built independently of Sage).

comment:6 Changed 7 years ago by zimmerma

I can reproduce the problem on a 32-bit Pentium 4, both with ecm-6.3 and the svn version. I will try to find a fix.

Paul

comment:7 Changed 7 years ago by zimmerma

I will try to find a fix.

with Pierrick Gaudry, we have analyzed and fixed the problem.

Long story: the asm volatile instructions without constraints in the configure script compile fine without -msse2, but those from the spv.c file, which have constraints, do not compile without -msse2. The fix was to put instructions with constraints in configure.

Short story: apply the following patch

--- configure.in        (revision 1545)
+++ configure.in        (revision 1546)
@@ -296,9 +296,9 @@
   AC_MSG_CHECKING([for SSE2 support])
   m4_define([SSE2_TEST_PROG], [AC_LANG_PROGRAM([], dnl
 [#if (defined(__GNUC__) || defined(__ICL)) && defined(__i386__)
-/* When there are no constraints, registers are referred to by
-   single % sign, not double. Argh */
-asm volatile ("pmuludq %xmm2, %xmm0");
+/* On some machines, a program without constraints may pass without -msse2 but
+   those with constraints in spv.c fail, thus we test with constraints here. */
+asm volatile ("pmuludq %%xmm2, %%xmm0" : : :"%xmm0");
 #else
 #error
 #IRIXdoesnotexitaterrordirective

comment:8 follow-ups: Changed 7 years ago by jdemeyer

I can imagine that this bug-fix will make the ecm package work, but I believe that the deeper issue in Sage still needs to be addressed: namely that spkg-install (both for mpir and for ecm) is providing CFLAGS when it should leave that to the packages themselves.

For example, on sage.math.washington.edu, the upstream mpir package uses the flags -O2 -m64 -march=core2 -mtune=core2 (i.e.: optimized for the particular machine) while within Sage, the flags -g -O3 are used.

comment:9 in reply to: ↑ 8 Changed 7 years ago by dimpase

Replying to jdemeyer:

I can imagine that this bug-fix will make the ecm package work, but I believe that the deeper issue in Sage still needs to be addressed: namely that spkg-install (both for mpir and for ecm) is providing CFLAGS when it should leave that to the packages themselves.

For example, on sage.math.washington.edu, the upstream mpir package uses the flags -O2 -m64 -march=core2 -mtune=core2 (i.e.: optimized for the particular machine) while within Sage, the flags -g -O3 are used.

How does the upstream mpir find these optimized flags?

comment:10 Changed 7 years ago by jdemeyer

  • Priority changed from blocker to major

comment:11 in reply to: ↑ 8 Changed 7 years ago by leif

Replying to jdemeyer:

I can imagine that this bug-fix will make the ecm package work, but I believe that the deeper issue in Sage still needs to be addressed: namely that spkg-install (both for mpir and for ecm) is providing CFLAGS when it should leave that to the packages themselves.

I consider such upstream issues, i.e., upstream should IMHO "modify" CFLAGS et al. if appropriate or necessary, rather than either ignoring them or (solely) using user-specified settings "as is".

(Try e.g. env CFLAGS="" ./sage -ba-force && ./sage -c "quit"... which breaks the Cython-generated C code, or at least used to do so.)


For example, on sage.math.washington.edu, the upstream mpir package uses the flags -O2 -m64 -march=core2 -mtune=core2 (i.e.: optimized for the particular machine) while within Sage, the flags -g -O3 are used.

Just for the record: -march=hobel implies -mtune=hobel (and -mcpu=hobel).

comment:12 follow-up: Changed 7 years ago by zimmerma

I consider such upstream issues, i.e., upstream should IMHO "modify" CFLAGS et al. if appropriate or necessary

I disagree. If the user requests CFLAGS=..., upstream should use those flags.

Paul Zimmermann

comment:13 in reply to: ↑ 12 Changed 7 years ago by leif

Replying to zimmerma:

I consider such upstream issues, i.e., upstream should IMHO "modify" CFLAGS et al. if appropriate or necessary

I disagree. If the user requests CFLAGS=..., upstream should use those flags.

Yes, but not (necessarily) exclusively. ;-)

(Almost all build scripts append or prepend to CFLAGS etc., which is convenient if one only wants to add e.g. "-g", "-march=native" or "-mcpu=i486". Here we want the former...)

comment:14 follow-ups: Changed 7 years ago by leif

  • Status changed from new to needs_info

So we at least still have the problem of adding -m64 if SAGE64=yes (i.e., if we want a 64-bit build on systems that default to 32-bit builds):

...
# Note that GMP-ECM is written in C (and assembler) - no C++ sources.

if [ "$SAGE64" = yes ]; then
    echo "Building a 64-bit version of GMP-ECM"
    if [ -z "$CFLAG64" ]; then
        CFLAG64=-m64
    fi
    CFLAGS="$CFLAGS $CFLAG64 -fPIC"
    LDFLAGS="$CFLAG64"; export LDFLAGS
else
    CFLAGS="$CFLAGS -fPIC"
fi

# We add debug symbols by default;
if [ "$SAGE_DEBUG" = yes ]; then
    # Disable optimization:
    CFLAGS="$CFLAGS -g -O0"
else
    # Enable optimization, may be overridden by user settings:
    CFLAGS="-g -O3 $CFLAGS"
fi

export CFLAGS # usually redundant, but safe(r)
...

Of course I could add "our" (or worse, also any user-specified) CFLAGS to CC, i.e. the C compiler command, and unset CFLAGS to make ECM's effective, but I wouldn't like that. (And some settings then would get overridden by ECM's CFLAGS anyway.)


An IMHO better solution is to also add -march=native to CFLAGS, since this enables SSE (to be precise, allows SSE2 instructions) if supported by the processor (when configure defines HAVE_SSE2), ignoring SAGE_FAT_BINARY at least for the moment (unless ECM's configure has an option like --disable-sse or similar; I haven't really checked that yet, perhaps just --enable-sse2=no on 32-bit x86).


I'm not sure what happens if we configure with CFLAGS set and --enable-gmp-cflags=no (which seems to be more appropriate when using ECM with MPIR).

comment:15 follow-up: Changed 7 years ago by leif

  • Status changed from needs_info to needs_work

Just for the record: I'll upload a new spkg that also fixes this issue at #5847 (hopefully) soon...

comment:16 in reply to: ↑ 14 Changed 7 years ago by zimmerma

Replying to leif:

So we at least still have the problem of adding -m64 if SAGE64=yes [...]

not sure. GMP uses 64-bit words if available (even if the system is 32 bit), and GMP-ECM uses GMP CFLAGS.

Paul

comment:17 follow-up: Changed 7 years ago by jdemeyer

So the solution is easy: don't set any CFLAGS in the mpir and ecm packages. Right?

comment:18 in reply to: ↑ 17 Changed 7 years ago by zimmerma

Replying to jdemeyer:

So the solution is easy: don't set any CFLAGS in the mpir and ecm packages. Right?

that is my advice. Same for MPFR, which also copies CFLAGS from GMP.

Paul

comment:19 in reply to: ↑ 14 ; follow-up: Changed 7 years ago by fbissey

Replying to leif:

An IMHO better solution is to also add -march=native to CFLAGS, since this enables SSE (to be precise, allows SSE2 instructions) if supported by the processor (when configure defines HAVE_SSE2), ignoring SAGE_FAT_BINARY at least for the moment (unless ECM's configure has an option like --disable-sse or similar; I haven't really checked that yet, perhaps just --enable-sse2=no on 32-bit x86).

You know that -march=native is only supported on x86 and amd64 cpus right? I can tell you first hand that the compilation will stop straight away on ppc. And then you will get Dave telling you it is not supported by other compilers.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 7 years ago by leif

Replying to fbissey:

You know that -march=native is only supported on x86 and amd64 cpus right?

Not only, but ...

I can tell you first hand that the compilation will stop straight away on ppc.

I was not going to try to enable SSE2 on PowerPCs (nor SPARC processors). ;-)


And then you will get Dave telling you it is not supported by other compilers.

There's very little support for other compilers in Sage, and it's easy to add a distinction when the day it gets necessary comes, though I could add it now.

comment:21 in reply to: ↑ 20 Changed 7 years ago by fbissey

Replying to leif:

Replying to fbissey:

You know that -march=native is only supported on x86 and amd64 cpus right?

Not only, but ...

I can tell you first hand that the compilation will stop straight away on ppc.

I was not going to try to enable SSE2 on PowerPCs (nor SPARC processors). ;-)


And then you will get Dave telling you it is not supported by other compilers.

There's very little support for other compilers in Sage, and it's easy to add a distinction when the day it gets necessary comes, though I could add it now.

Ok, I missed a little bit of context. I guess I didn't pay full attention to the whole thread. Sorry for the noise.

comment:22 in reply to: ↑ 15 ; follow-up: Changed 7 years ago by leif

  • Description modified (diff)
  • Report Upstream changed from N/A to Fixed upstream, but not in a stable release.
  • Status changed from needs_work to needs_review

Replying to leif:

Just for the record: I'll upload a new spkg that also fixes this issue at #5847 (hopefully) soon...

Did so...

Setting this ticket to "needs review" too, can be closed as duplicate when #5847 gets a positive review / merged.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 7 years ago by leif

Replying to leif:

Replying to leif:

Just for the record: I'll upload a new spkg that also fixes this issue at #5847 (hopefully) soon...

Did so...

Setting this ticket to "needs review" too, can be closed as duplicate when #5847 gets a positive review / merged.

I've updated the p2 package at #5847 (some corrections, some improvements); same place, different md5sum, still or again needing testing (especially on PowerPCs) / review.

See http://trac.sagemath.org/sage_trac/ticket/5847#comment:86 .

comment:24 in reply to: ↑ 23 Changed 7 years ago by dimpase

Replying to leif:

Replying to leif:

Replying to leif:

Just for the record: I'll upload a new spkg that also fixes this issue at #5847 (hopefully) soon...

Did so...

Setting this ticket to "needs review" too, can be closed as duplicate when #5847 gets a positive review / merged.

I've updated the p2 package at #5847 (some corrections, some improvements); same place, different md5sum, still or again needing testing (especially on PowerPCs) / review.

See http://trac.sagemath.org/sage_trac/ticket/5847#comment:86 .

duly tested on PPC, see the other ticket.

comment:25 follow-up: Changed 7 years ago by robertwb

  • Milestone changed from sage-4.6.1 to sage-duplicate/invalid/wontfix
  • Status changed from needs_review to needs_info

Is there any reason this shouldn't be closed as a dup now?

comment:26 in reply to: ↑ 25 Changed 7 years ago by leif

Replying to robertwb:

Is there any reason this shouldn't be closed as a dup now?

Well, it's a symbolic link to #5847, which still needs review, delaying the MPIR upgrade (#8664) further.

We should close it when #5847 got merged.

comment:27 Changed 6 years ago by leif

  • Dependencies set to #5847
  • Description modified (diff)
  • Keywords SSE2 added
  • Status changed from needs_info to needs_review

comment:28 Changed 6 years ago by leif

  • Status changed from needs_review to positive_review

comment:29 Changed 6 years ago by leif

  • Reviewers set to Leif Leonhardy

comment:30 follow-up: Changed 6 years ago by zimmerma

for your information, this bug is fixed upstream, but we have no new release yet. The fix consists in adding -msse2 when required to compile the corresponding assembly files. See around lines 297-326 of the configure.in file in the svn version upstream.

Paul Zimmermann

comment:31 in reply to: ↑ 30 Changed 6 years ago by leif

Replying to zimmerma:

for your information, this bug is fixed upstream, but we have no new release yet.

Thanks. We already apply that patch (I think; from revision 1546) in the .p2 spkg at #5847.

comment:32 Changed 6 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to duplicate
  • Status changed from positive_review to closed

comment:33 Changed 6 years ago by jdemeyer

  • Merged in sage-4.7.2.alpha3 deleted

comment:34 Changed 5 years ago by zimmerma

this problem is fixed in GMP-ECM 6.4, which has just been released.

Paul Zimmermann

Note: See TracTickets for help on using tickets.