Opened 2 years ago

Closed 2 years ago

#12830 closed enhancement (fixed)

Work around GCC 4.7.0 bug on ia64 and improve the GMP-ECM spkg

Reported by: leif Owned by: leif
Priority: blocker Milestone: sage-5.0
Component: packages: standard Keywords: spkg -march=native assembler error Darwin MacOS __GMP_CFLAGS __MPIR_CFLAGS gmp.h GCC 4.7.0 ia64 Itanium bug impossible reload
Cc: hedtke, justin Merged in: sage-5.0.rc0
Authors: Leif Leonhardy Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by leif)

GCC 4.7.0 is broken on ia64 (Itanium), which also affects the GMP-ECM spkg (cf. #12751).


Adding -march=native to CFLAGS may lead to assembler errors, e.g. on MacOS X with newer GCCs on newer CPUs (e.g. such supporting AVX, which Apple's assembler currently doesn't).

Newer versions of MPIR don't define __GMP_CFLAGS (in gmp.h) to a string literal, but instead to a preprocessor macro (__MPIR_CFLAGS), which in turn is defined to the string we want.


New spkg: http://boxen.math.washington.edu/home/leif/Sage/spkgs/ecm-6.3.p7.spkg

md5sum: a5e9a4b47d30c4aba2c77ada47579e1a ecm-6.3.p7.spkg

ecm-6.3.p7 (Leif Leonhardy, April 19th 2012)

  • #12830: Add -m[no-]power* to the processor-specific options recognized; these are used to enable/disable specific instruction set extensions of the POWER and PowerPC (PPC) CPUs.

ecm-6.3.p6 (Leif Leonhardy, April 16th 2012)

  • #12830: Add a work-around for GCC 4.7.x on ia64 (Itanium), since GMP-ECM currently won't build with that and anything but -O0 on that platform.
  • Use \{1,\} instead of \+ in sed patterns, which is more portable.
  • Also support newer system-wide MPIR installations for printing their settings.
  • Use patch to apply patches. Since the pre-patched configure in patches/ was created with a newer version of autotools (or, rather, the original configure was created with an outdated version), the patch would have been almost as large as the patched configure file itself. Hence I autoreconfed the source tree with a patched configure.in (and almost the latest versions of autotools), then created a patch to configure from the resulting file(s). Note that therefore src/ isn't really vanilla any more, although just the auto-generated files differ (which are still made from vanilla upstream sources, including configure.in). Add a "Patches" subsection and update "Special Update/Build? Instructions". Remove files in patches/ from .hgignore (and also remove pre-patched files from that directory); the patch to configure and the diff of configure.in are [now] under revision control, which IMHO makes sense.
  • Beautify (and simplify) the output with respect to options passed to configure; print the settings of a few more environment variables that GMP-ECM uses (in case they're set); add some messages, also mention --enable-assert etc. if SAGE_DEBUG=yes.
  • Remove unused test for GCC.

ecm-6.3.p5 (Leif Leonhardy, April 11th 2012)

  • #12830: Don't add -march=native if the assembler doesn't understand the instructions the compiler emits with that. (E.g. the Apple/XCode assembler on Darwin doesn't yet support AVX.)
  • Fix extraction of __GMP_CC and __GMP_CFLAGS (from gmp.h) for newer versions of MPIR, which define these to preprocessor macros rather than strings.
  • Redirect warnings and error messages to stderr; add some messages.
  • Correct SPKG.txt w.r.t. applied patches.

Attachments (4)

ecm-6.3.p4-p5.diff (10.7 KB) - added by leif 2 years ago.
Diff between the previous spkg in Sage and my new p5 spkg. For reference / review only.
ecm-6.3.p5-p6.diff (17.7 KB) - added by leif 2 years ago.
Diff between my p5 and p6 spkgs. For reference / review only.
ecm-6.3.p4-p6.diff (23.8 KB) - added by leif 2 years ago.
Diff between the previous spkg in Sage and my new p6 spkg. For reference / review only.
ecm-6.3.p6-p7.diff (1.7 KB) - added by leif 2 years ago.
Diff between my p6 and p7 spkgs. For reference / review only.

Download all attachments as: .zip

Change History (33)

Changed 2 years ago by leif

Diff between the previous spkg in Sage and my new p5 spkg. For reference / review only.

comment:1 Changed 2 years ago by leif

  • Authors set to Leif Leonhardy
  • Cc hedtke justin added
  • Description modified (diff)
  • Status changed from new to needs_review

This new spkg (also) fixes issues (i.e., assembler errors) on MacOS X with the GCC spkg.

Please test and review!

comment:2 follow-up: Changed 2 years ago by jdemeyer

Instead of copying all this complicated "detect CFLAGS from MPIR" stuff, I propose the following: make the MPIR spkg store its $CC and $CFLAGS somewhere (say, in $SAGE_LOCAL/etc/mpirflags) and read that? That would replace the horrible kludge we have now in this ecm spkg.

comment:3 follow-up: Changed 2 years ago by jdemeyer

  1. Remove the reading of the system GMP header, as they aren't used anyway (there is no good reason to use them anyway).
  1. Remove unset RM which isn't needed anymore.

comment:4 follow-up: Changed 2 years ago by jdemeyer

Concerning march=native: what do you think about using testcflags.sh for this? Let's add

unsigned long fancy_insns() { return (unsigned long) d; }

to the program that testcflags.sh uses to check flags.

Then we could do something like

if testcflags.sh -march=native >/dev/null; then
    CFLAGS="-march=native $CFLAGS"
elif testcflags.sh "-march=native -mno-avx" >/dev/null; then
    echo >&2 "Warning bla bla"
    CFLAGS="-march=native -mno-avx $CFLAGS"
fi

comment:5 in reply to: ↑ 2 ; follow-up: Changed 2 years ago by leif

Replying to jdemeyer:

Instead of copying all this complicated "detect CFLAGS from MPIR" stuff, I propose the following: make the MPIR spkg store its $CC and $CFLAGS somewhere (say, in $SAGE_LOCAL/etc/mpirflags) and read that? That would replace the horrible kludge we have now in this ecm spkg.

Well, using gmp.h is the documented, supposed way to do this, and compatible to GMP.

Extracting the flags isn't complicated (nor a kludge); complain to the MPIR developers that they break GMP compatibility even with --enable-gmpcompat (as I already did). The extra code working around this is six lines IIRC, and not really complicated.


If you want MPIR and GMP to store the flags elsewhere, suggest them to create a pkg-config file (although "standard" .pc files don't have a variable for the compiler used).

We could of course create our own in MPIR's (and GMP's) spkg-install, but that doesn't make much sense to me, since then again our other spkgs using the .pc file would depend on Sage's versions (and a recent version of Sage itself).

comment:6 in reply to: ↑ 3 Changed 2 years ago by leif

Replying to jdemeyer:

  1. Remove the reading of the system GMP header, as they aren't used anyway (there is no good reason to use them anyway).

It's for informational purposes; it's not bad to compare (potentially) MPIR's flags to those of a system-wide GMP (or MPIR) installation.

  1. Remove unset RM which isn't needed anymore.

It doesn't hurt, and doesn't break anything.

On the contrary, removing it may break the installation of the spkg on older versions of Sage.

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

Replying to jdemeyer:

Concerning march=native: what do you think about using testcflags.sh for this? Let's add

unsigned long fancy_insns() { return (unsigned long) d; }

to the program that testcflags.sh uses to check flags.

Then we could do something like

if testcflags.sh -march=native >/dev/null; then
    CFLAGS="-march=native $CFLAGS"
elif testcflags.sh "-march=native -mno-avx" >/dev/null; then
    echo >&2 "Warning bla bla"
    CFLAGS="-march=native -mno-avx $CFLAGS"
fi

Probably. Although testcflags.sh is currently broken... ;-)

I'm not sure if we should do such a complicated test whenever some arbitrary compiler flag is to be tested. We'd never know whether the compiler, the assembler or the linker caused an error.

Using it again requires newer versions of Sage for little reason, which I don't like in general.

testcflags.sh might have an option to not use CFLAGS btw.; otherwise one would have to (unset CFLAGS; testcflags.sh ...) or specifically construct CFLAGS for the desired purpose.

comment:8 in reply to: ↑ 7 Changed 2 years ago by jdemeyer

Replying to leif:

Probably. Although testcflags.sh is currently broken... ;-)

In which sense is it broken???

I'm not sure if we should do such a complicated test whenever some arbitrary compiler flag is to be tested. We'd never know whether the compiler, the assembler or the linker caused an error.

Why does it matter whether the compiler, assembler or linker caused the problem? What matters is that it doesn't work.

Using it again requires newer versions of Sage for little reason, which I don't like in general.

That I noticed :-)

Using testcflags.sh looks like the cleaner and simpler solution to me.

testcflags.sh might have an option to not use CFLAGS btw.; otherwise one would have to (unset CFLAGS; testcflags.sh ...) or specifically construct CFLAGS for the desired purpose.

I think

CFLAGS= testcflags.sh -flag

is simple enough.

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

What's wrong with simply storing MPIR's flags in some easy-to-read file? That would be the most simple solution. Moreover, it would ensure that we don't need to change the ECM spkg (or any other) if either Sage or MPIR changes the CFLAGS handling for MPIR. We would only need to change the MPIR spkg.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 2 years ago by leif

Replying to jdemeyer:

What's wrong with simply storing MPIR's flags in some easy-to-read file? That would be the most simple solution.

Why reinvent the wheel?

(And reading gmp.h isn't complicated at all, presumably because doing so is intended...)


Moreover, it would ensure that we don't need to change the ECM spkg (or any other) if either Sage or MPIR changes the CFLAGS handling for MPIR. We would only need to change the MPIR spkg.

I don't see the point. First of all, that would be incompatible to any other MPIR or GMP installation / package, and we'd certainly have to change all packages using such a file whenever we change "Sage's format" (or handling) of MPIR's flags / settings.

comment:11 in reply to: ↑ 10 Changed 2 years ago by jdemeyer

Replying to leif:

Replying to jdemeyer:

What's wrong with simply storing MPIR's flags in some easy-to-read file? That would be the most simple solution.

Why reinvent the wheel?

You are already reinventing the wheel by manually sedding the flags out of the header file, as opposed to using autoconf. So if we reinvent the wheel, let's reinvent it as simple as possible.

comment:12 Changed 2 years ago by jdemeyer

Besides, your wheel is broken as \+ isn't portable.

Version 0, edited 2 years ago by jdemeyer (next)

comment:13 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Changed 2 years ago by leif

Diff between my p5 and p6 spkgs. For reference / review only.

Changed 2 years ago by leif

Diff between the previous spkg in Sage and my new p6 spkg. For reference / review only.

comment:14 Changed 2 years ago by leif

  • Description modified (diff)
  • Keywords GCC 4.7.0 ia64 Itanium bug impossible reload added
  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_work to needs_review
  • Summary changed from Improve handling of CFLAGS in GMP-ECM's spkg-install to Work around GCC 4.7.0 bug on ia64 and improve the GMP-ECM spkg

I've made a p6 spkg with further fixes and improvements, now also working around the GCC 4.7.0 bug on ia64 (Itanium).

(See attached diffs and the spkg changelog entry in the description for details.)

comment:15 Changed 2 years ago by jdemeyer

  • Priority changed from major to blocker

comment:16 follow-up: Changed 2 years ago by jdemeyer

In your case statement:

-march=*|-mcpu=*|-mtune*)

add -mpower* which MPIR also uses.

Of course, I still think the spkg-install is overly complicated and re-invents too many wheels. But since you don't want to give in, I guess I'll have to, otherwise this will never get merged.

comment:17 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:18 in reply to: ↑ 16 Changed 2 years ago by leif

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Replying to jdemeyer:

In your case statement:

-march=*|-mcpu=*|-mtune*)

add -mpower* which MPIR also uses.

Fixed in a p7. (I've added -mpower* and -mno-power*.)

comment:19 Changed 2 years ago by leif

  • Description modified (diff)

Changed 2 years ago by leif

Diff between my p6 and p7 spkgs. For reference / review only.

comment:20 follow-up: Changed 2 years ago by ppurka

Is egrep available on all systems? Otherwise one can use grep -E.

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

Replying to ppurka:

Is egrep available on all systems? Otherwise one can use grep -E.

egrep is the more portable of these two: Solaris (at least some versions) does support egrep but not grep -E.

comment:22 in reply to: ↑ 21 Changed 2 years ago by leif

Replying to jdemeyer:

Replying to ppurka:

Is egrep available on all systems? Otherwise one can use grep -E.

egrep is the more portable of these two: Solaris (at least some versions) does support egrep but not grep -E.

And we do use egrep in a couple of other spkgs (and probably scripts, too) since quite a while by the way.

In general, the safest way would be to use some $EGREP, determined by Sage's configure, but that's a different story...

comment:23 Changed 2 years ago by vbraun

  • Status changed from needs_review to positive_review

Looks fine.

In the long run it is probably unavoidable to disable optimizations on ia64 as nobody is going to fix bugs in the itanium optimizer at this point in time.

comment:24 follow-up: Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

As reported on https://groups.google.com/d/msg/sage-devel/F8Y6DP3eSLM/zQM7HKzLt68J, this fails on SLES10 x86_64 with

libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I./x86_64 
-I/home/rajeev/bin/sage-5.0.beta14/local/include 
-I/home/rajeev/bin/sage-5.0.beta14/local/include -march=native -g -O3 
-fPIC -MT libecm_la-mul_fft.lo -MD -MP -MF .deps/libecm_la-mul_fft.Tpo 
-c mul_fft.c -o libecm_la-mul_fft.o 
: Assembler messages: 
:16797: Error: no such instruction: `pmulld %xmm2,%xmm0' 
:1023: Error: no such instruction: `pmulld %xmm2,%xmm0' 
:608: Error: no such instruction: `pmulld %xmm2,%xmm0' 
make[4]: *** [libecm_la-mul_fft.lo] Error 1 

This seems to be a known feature of gcc -march=native sometimes emitting SSE4 asm that is not supported by the installed binutils. See also http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32062

comment:25 Changed 2 years ago by jdemeyer

It seems that MPIR is quite clever about choosing CFLAGS. So, as I have said before, I would simply use MPIR's CFLAGS. My feeling is that we should still merge this spkg as-is in sage-5.0 and continue thinking about it for the next Sage release.

comment:26 in reply to: ↑ 24 Changed 2 years ago by jdemeyer

Replying to vbraun:

This seems to be a known feature of gcc -march=native sometimes emitting SSE4 asm that is not supported by the installed binutils.

I don't think that gcc ever checks what the assembler supports. It just emits instructions and assumes that the assembler knows about them.

comment:27 follow-up: Changed 2 years ago by vbraun

I agree, gcc just assumes that sufficiently recent binutils are installed. So we really would need to also bundle binutils and its required BFD and opcodes libraries, plus whatever else they use. It was probably a mistake to use a cutting-edge gcc binary, we should use one that is as old as possible while having all critical bugs fixed. Or have some framework for CFLAGS that is smart enough to not turn on SSE levels that the assembler does not support.

I'm fine with closing this ticket and fixing the SLES problems in the next release (and on another ticket).

comment:28 in reply to: ↑ 27 Changed 2 years ago by jdemeyer

Replying to vbraun:

we should use one that is as old as possible while having all critical bugs fixed.

That's exactly what I did :-)

comment:29 Changed 2 years ago by jdemeyer

  • Merged in set to sage-5.0.rc0
  • Resolution set to fixed
  • Status changed from needs_work to closed
Note: See TracTickets for help on using tickets.