Opened 8 years ago

Closed 8 years ago

#11574 closed enhancement (fixed)

update M4RI to newest upstream release

Reported by: malb Owned by:
Priority: critical Milestone: sage-4.7.2
Component: packages: standard Keywords: sd32
Cc: strogdon, leif, AlexanderDreyer Merged in: sage-4.7.2.alpha3
Authors: Martin Albrecht Reviewers: Simon King, Alexander Dreyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11261 #11756 Stopgaps:

Attachments (2)

trac_11574_m4ri_sse2.patch (3.7 KB) - added by malb 8 years ago.
m4ri_20110601.patch (10.0 KB) - added by malb 8 years ago.
rebased to 4.7.1.alpha4

Download all attachments as: .zip

Change History (82)

comment:1 Changed 8 years ago by malb

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

comment:2 Changed 8 years ago by malb

  • Dependencies set to #11261

comment:3 Changed 8 years ago by kcrisman

I'm a little confused. The spkgs here and at #9562 are both libm4ri, and seem to be numbered backwards (the "earlier" one in the dependency is from 2011).

comment:4 Changed 8 years ago by kcrisman

Okay, I think I figured it out. Sorry for the noise.

comment:5 Changed 8 years ago by kcrisman

I don't know what I did wrong, but I still get the problem with the Symbol not found: _m4ri_swap_bits while trying to start, even after this patch. I may have done something wrong, but at any rate I can't spend more time on this now, sorry.

comment:6 Changed 8 years ago by malb

  • Authors set to Martin Albrecht
  • Description modified (diff)

comment:7 Changed 8 years ago by malb

For the record, I tried spkg + patch on t2 and doctests pass.

comment:8 Changed 8 years ago by malb

  • Description modified (diff)

comment:9 Changed 8 years ago by malb

applies & passes doctests with 4.7.1.rc2.

comment:10 Changed 8 years ago by SimonKing

  • Reviewers set to Simon King
  • Status changed from needs_review to needs_work

Why is the documentation for matrices over GF(2) not part of the reference manual? I think you should use the upgrade to expose the documentation to the audience.

comment:11 Changed 8 years ago by malb

I am not sure this documentation is much use in the reference manual since most of the interesting functions won't show up. Also, these matrices are expected to behave just as other matrices. On the other hand, there are a few - not many - special functions which are only available for GF(2). I'll take a look.

comment:12 Changed 8 years ago by malb

  • Status changed from needs_work to needs_review

It seems that you agreed in a different ticket (#9562) that this class doesn't need to be in the reference manual? Hence, returning to needs review. Feel free to change it back, though.

comment:13 Changed 8 years ago by SimonKing

I think I can live with not including it into the references. However, currently I am getting segment faults when doctesting on mark (one of the skynet machines). I try to make sure that I properly install everything from scratch again, before reporting details.

comment:14 Changed 8 years ago by SimonKing

I did a forced new installation of the polybori spkg, then of the M4RI spkg, and I also installed the patches. I get on mark:

sage: P.<a,b,c,d,e> = PolynomialRing(GF(2), 5, order='lex')
sage: I1 = ideal([a*b + c*d + 1, a*c*e + d*e, a*b*e + c*e, b*c + c*d*e + 1])
sage: B.<a,b,c,d,e> = BooleanPolynomialRing(5, order='lex')
sage: I2 = ideal([B(f) for f in I1.gens()])
sage: I2.groebner_basis()
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)

/home/simonking/SAGE/sage-4.7.1.rc1mark/devel/sage-main/<ipython console> in <module>()

/home/simonking/SAGE/sage-4.7.1.rc1mark/local/lib/python2.6/site-packages/sage/rings/polynomial/pbori.so in sage.rings.polynomial.pbori.BooleanPolynomialIdeal.groebner_basis (sage/rings/polynomial/pbori.cpp:26221)()

RuntimeError: Segmentation fault

Of course, it could be that the problem comes from the polybori spkg. That would be unfortunate, since it is already merged.

comment:15 Changed 8 years ago by SimonKing

Now, I tried to force re-installation of the original polybori spkg. However, that failed as well. I hope that it will not be needed to rebuild all of sage, which takes ages on mark.

comment:16 Changed 8 years ago by malb

I think the issue is that one needs to install stuff in the right order:

  • install new M4RI
  • install new PolyBoRi?
  • apply the M4RI patch
  • touch sage/libs/polybori/decl.pxd
  • sage -b

comment:17 follow-up: Changed 8 years ago by SimonKing

Meanwhile I managed to revert to the original state, and can confirm that, with vanilla sage-4.7.1.rc2 on 32-bit solaris, the segfault above does not occur.

I will now try to do things in the order you suggested: In fact, originally I had installed polybori, then m4ri, then the m4ri patch, and then sage -b (without touching decl.pxd).

But how can the new polybori spkg be merged if apparently it depends on the new m4ri?

comment:18 Changed 8 years ago by SimonKing

The new installation order did the trick, the segfault has gone. I am now repeating the failing doctests.

comment:19 Changed 8 years ago by SimonKing

Or actually I am repeating sage -testall.

comment:20 in reply to: ↑ 17 Changed 8 years ago by malb

Replying to SimonKing:

But how can the new polybori spkg be merged if apparently it depends on the new m4ri?

Hi, the PolyBoRi? does not depend on the new M4RI. This version of M4RI breaks binary compatibility with previous M4RI, i.e. we changed how matrices look internally. Hence, for many pieces of software linking against M4RI one needs to rebuild them to make sure they are talking to the right M4RI library in the right way. That's why M4RI and the PolyBoRi? is necessary: to tell PolyBoRi? to use this new version. I think we need to updated PolyBoRi? as well because some macro names changed and PolyBoRi? 0.7.1.p4 contains the necessary code to deal with it.

comment:21 Changed 8 years ago by SimonKing

  • Status changed from needs_review to positive_review

The tests took 84373.7 seconds on mark, and there were a couple of timeouts. However, there was no failure. The segfault that I reported above vanished by installing everything in the right order. On my computer, all tests pass.

Moreover, it is a prerequisite for working with M4RIE (#9562), which I think is a very important contribution - currently, matrices over GF(2e) are painfully slow in Sage.

I think it is ok that the matrices over GF(2) do not have their own section in the reference manual, since there seems that all non-underscore methods are already present for general matrices.

Hence, it is a positive review.

comment:22 Changed 8 years ago by malb

Woot!

comment:23 Changed 8 years ago by was

  • Priority changed from major to critical

I'm upping the priority, since this fixes a serious mathematical bug reported on sage-support.

comment:24 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

The directory m4ri should be added to .hgignore

comment:25 Changed 8 years ago by malb

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

the updated SPKG fixes this issue.

comment:26 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.2.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:27 follow-up: Changed 8 years ago by jdemeyer

  • Merged in sage-4.7.2.alpha2 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

On some 32-bit Linux systems (for example, the buildbot machine cicero), this causes polybori to fail to install:

g++ -o groebner/src/randomset.o -c -O3 -Wno-long-long -Wreturn-type -g -fPIC -ftemplate-depth-100 -O3 -Wno-long-long -Wreturn-type -g -fPIC -DNDEBUG -DHAVE_GD -DHAVE_TR1_UNORDERED_MAP -DPACKED -DHAVE_M4RI -DHAVE_GD -DHAVE_IEEE_754 -DBSD -I/home/jdemeyer/sage-4.7.2.alpha1/local/include -I/home/jdemeyer/sage-4.7.2.alpha1/local/include/python2.6 -Ipolybori/include -ICudd/obj -ICudd/util -ICudd/cudd -ICudd/mtr -ICudd/st -ICudd/epd groebner/src/randomset.cc
In file included from /home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:42,
                 from /home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/permutation.h:31,
                 from /home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/m4ri.h:50,
                 from groebner/src/nf.h:15,
                 from groebner/src/randomset.cc:11:
/usr/lib/gcc/i686-pc-linux-gnu/4.4.3/include/emmintrin.h:32:3: error: #error "SSE2 instruction set not enabled"
In file included from /home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/permutation.h:31,
                 from /home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/m4ri.h:50,
                 from groebner/src/nf.h:15,
                 from groebner/src/randomset.cc:11:
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h: In function 'void mzd_row_add_offset(mzd_t*, rci_t, rci_t, rci_t)':
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:741: error: '__m128i' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:741: error: '__src' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:741: error: expected primary-expression before ')' token
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:741: error: expected ';' before 'src'
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:742: error: '__dst' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:742: error: expected primary-expression before ')' token
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:742: error: expected ';' before 'dst'
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:743: error: expected primary-expression before 'const'
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:743: error: expected ';' before 'const'
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:746: error: expected ';' before 'xmm1'
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:747: error: 'xmm1' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:749: error: 'eof' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h: In function 'void mzd_combine_even_in_place(mzd_t*, rci_t, wi_t, const mzd_t*, rci_t, wi_t)':
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1190: error: '__m128i' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1190: error: 'a128' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1190: error: expected primary-expression before ')' token
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1190: error: expected ';' before 'a'
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1191: error: 'b128' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1191: error: expected primary-expression before ')' token
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1191: error: expected ';' before 'b'
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1192: error: expected initializer before '*' token
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1195: error: '_mm_xor_si128' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1198: error: 'eof' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h: In function 'void mzd_combine_even(mzd_t*, rci_t, wi_t, const mzd_t*, rci_t, wi_t, const mzd_t*, rci_t, wi_t)':
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1274: error: '__m128i' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1274: error: 'a128' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1274: error: expected primary-expression before ')' token
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1274: error: expected ';' before 'a'
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1275: error: 'b128' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1275: error: expected primary-expression before ')' token
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1275: error: expected ';' before 'b'
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1276: error: 'c128' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1276: error: expected primary-expression before ')' token
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1276: error: expected ';' before 'c'
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1277: error: expected initializer before '*' token
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1280: error: '_mm_xor_si128' was not declared in this scope
/home/jdemeyer/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1284: error: 'eof' was not declared in this scope
scons: *** [groebner/src/randomset.o] Error 1
scons: building terminated because of errors.

comment:28 Changed 8 years ago by jdemeyer

  • Status changed from new to needs_review

comment:29 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:30 follow-up: Changed 8 years ago by malb

Can you allow me to read your build on cicero (my username is malb). I don't have a copy of Sage on cicero and it would save me some time.

comment:31 Changed 8 years ago by was

  • Keywords sd32 added

comment:32 in reply to: ↑ 30 Changed 8 years ago by jdemeyer

Replying to malb:

Can you allow me to read your build on cicero (my username is malb). I don't have a copy of Sage on cicero and it would save me some time.

It's in /home/buildbot/build/sage/cicero-1/cicero_full/build/sage-4.7.2.alpha2 (should be accessible through NFS on any Skynet machine).

comment:33 in reply to: ↑ 27 ; follow-up: Changed 8 years ago by AlexanderDreyer

Replying to jdemeyer:

On some 32-bit Linux systems (for example, the buildbot machine cicero), this causes polybori to fail to install:

[...] Here on my system the old m4ri/config.h stays when the new libm4ri spkg is installed. Maybe something around polybori picks the outdated file?

Regards,

Alexander

comment:34 in reply to: ↑ 33 ; follow-up: Changed 8 years ago by AlexanderDreyer

Replying to AlexanderDreyer:

Here on my system the old m4ri/config.h stays when the new libm4ri spkg is installed. Maybe something around polybori picks the outdated file?

As Jeroen pointed out this is not related. Unfortunately I cannot reproduce this behaviour. Is there some machine on which this occurs, which I may be granted access to?

comment:35 in reply to: ↑ 34 ; follow-up: Changed 8 years ago by malb

Replying to AlexanderDreyer:

As Jeroen pointed out this is not related. Unfortunately I cannot reproduce this behaviour. Is there some machine on which this occurs, which I may be granted access to?

The only machine which I know of is 'cicero' on skynet. We could try to get you an account there but it would take a while I guess. Right now, I cannot even log into the machine.

comment:36 in reply to: ↑ 35 Changed 8 years ago by AlexanderDreyer

Replying to malb:

The only machine which I know of is 'cicero' on skynet. We could try to get you an account there but it would take a while I guess. Right now, I cannot even log into the machine.

This would be nice. I'm about to prepare a qemu image for Gentoo x86. But that will last for some time since I have to compile everything from scratch (It's Gentoo.)

comment:37 follow-up: Changed 8 years ago by fbissey

  • Cc strogdon added

I think thing go south from there

/usr/lib/gcc/i686-pc-linux-gnu/4.4.3/include/emmintrin.h:32:3: error: #error "SSE2 instruction set not enabled"

I had a x86 box without sse2 but it is now collecting dust without an OS installed. I will take a look on the closest thing I can get. Actually Steve Trogdon could have a box where this could be tested, I am copying him.

comment:38 in reply to: ↑ 37 Changed 8 years ago by strogdon

Replying to fbissey:

I think thing go south from there

/usr/lib/gcc/i686-pc-linux-gnu/4.4.3/include/emmintrin.h:32:3: error: #error "SSE2 instruction set not enabled"

I had a x86 box without sse2 but it is now collecting dust without an OS installed. I will take a look on the closest thing I can get. Actually Steve Trogdon could have a box where this could be tested, I am copying him.

I get the same failure as comment:33

/storage/strogdon/gentoo/usr/lib/gcc/i686-pc-linux-gnu/4.5.2/include/emmintrin.h:32:3: error: #error "SSE2 instruction set not enabled"
In file included from /storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/permutation.h:31:0,
                 from /storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/m4ri.h:50,
                 from groebner/src/nf.h:15,
                 from groebner/src/randomset.cc:11:

...

scons: *** [groebner/src/randomset.o] Error 1
scons: building terminated because of errors.
Error building PolyBoRi.

comment:39 Changed 8 years ago by malb

M4RI *should* test for SSE2 and only include that header if it is present. Since it worked so far, I assume it still does that. However, we recently switched from HAVE_SSE2 to __M4RI_HAVE_SSE2 and this might be a change that PolyBoRi? didn't pick up? However, it shouldn't be PolyBoRi?'s responsibility to check for SSE2 and hence something goes wrong when including m4ri.h.

comment:40 Changed 8 years ago by malb

For some reason m4ri_config.h on cicero has

#define __M4RI_HAVE_SSE2                1

which is totally wrong! I'll investigate.

comment:41 Changed 8 years ago by malb

More data on cicero:

cat /proc/cpuinfo reports SSE2

flags           : fpu vme de pse tsc msr pae mce cx8 apic mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe up pebs bts

but /usr/lib/gcc/i686-pc-linux-gnu/4.4.3/include/emmintrin.h:32:3 complains. I assume this is because neither PolyBoRi nor Sage pass -msse2 when linking against M4RI but -msse2 defines __SSE2__ which is required by emmintrin.h. But I don't understand yet why the previous version works.

comment:42 follow-up: Changed 8 years ago by malb

I can confirm that passing -msse2 fixes the issue. I guess I'll have to find a way to save the flags (to disc?) such that third parties (PolyBoRi, Sage) can re-use them? Any suggestions on how to do that?

comment:43 in reply to: ↑ 42 Changed 8 years ago by AlexanderDreyer

Replying to malb:

I can confirm that passing -msse2 fixes the issue. I guess I'll have to find a way to save the flags (to disc?) such that third parties (PolyBoRi, Sage) can re-use them? Any suggestions on how to do that?

For scons-bases builds (PolyBoRi as well as Sage) I should be able to provide a suitable code snipplet which checks whether M4RI_HAVE_SSE2 is defined in m4ri/m4ri_config.h.

comment:44 Changed 8 years ago by AlexanderDreyer

  • Owner changed from tbd to (none)

We have added some configuration for this here: https://bitbucket.org/brickenstein/polybori/changeset/e5ecbbe5a904 I'll backport this to 0.7.1 and bundle a new spkg asap.

comment:45 Changed 8 years ago by AlexanderDreyer

PS: I'm also checking things like sse3 which m4ri might or might not use in the future.

comment:46 follow-up: Changed 8 years ago by malb

Hi Alexander,

so your plan is to parse for __M4RI_HAVE_SSE2=1 and then pass -msse2 manually? The only caveat I could see with that approach is that it limits us to use !GCC. For Sage this isn't really a problem. M4RI supports more compilers. How's that for PolyBoRi?

If we want to support more compilers I was thinking about dumping the CFLAGS to disk in M4RI which third party tools then can re-use. I was thinking of using the pkg-config.rc file which one can then either use via pkg-config or parse oneself.

PS: Sage's main build scripts are not !Scons based, but I can certainly take care of those.

comment:47 Changed 8 years ago by malb

  • Cc leif added

comment:48 in reply to: ↑ 46 Changed 8 years ago by AlexanderDreyer

Replying to malb:

The only caveat I could see with that approach is that it limits us to use !GCC. For Sage this isn't really a problem. M4RI supports more compilers. How's that for PolyBoRi?

We support only gcc yet. In the case that we would add additional ones we'll add a mapping of non-default compiler options anyway. (e.g. -msse2 -> -xarch-sse2 for sun studio). But if you could export the original parameters, this would of course be better.

If we want to support more compilers I was thinking about dumping the CFLAGS to disk in M4RI which third party tools then can re-use. I was thinking of using the pkg-config.rc file which one can then either use via pkg-config or parse oneself.

I would prefer if you could add a macro defining the options as a string literal to m4ri_config.h. But if you need to setup the rc file anyway, you do not have to duplicate work.

comment:49 Changed 8 years ago by leif

Replying to malb:

I can confirm that passing -msse2 fixes the issue. I guess I'll have to find a way to save the flags (to disc?) such that third parties (PolyBoRi, Sage) can re-use them? Any suggestions on how to do that?

Take a look at GMP's / MPIR's gmp.h and the various libs (like e.g. MPFR) that use it:

$ egrep -B1 "__GMP_(CC|CFLAGS)" /usr/include/gmp.h
/* Define CC and CFLAGS which were used to build this version of GMP */
#define __GMP_CC "gcc -std=gnu99"
#define __GMP_CFLAGS "-march=native -g -O3"

I for example use (slightly OT):

# We eventually use / add these settings (or parts of them) to "our" CFLAGS,
# e.g. "-march=..." or "-mcpu=..." and "-mtune=..." to let gcc generate better,
# processor-specific code, since ECM doesn't use them if we set our own:
gmp_cc_pat='/^[         ]*#[    ]*define[       ]\+__GMP_CC[    ]\+/s/.*"\([^"]*\)"/\1/p'
gmp_cflags_pat='/^[     ]*#[    ]*define[       ]\+__GMP_CFLAGS[        ]\+/s/.*"\([^"]*\)"/\1/p'

gmp_cc=`sed -n -e "$gmp_cc_pat" "$SAGE_LOCAL"/include/gmp.h`
gmp_cflags=`sed -n -e "$gmp_cflags_pat" "$SAGE_LOCAL"/include/gmp.h`

system_gmp_h=""
for incdir in /usr/include /usr/local/include; do
    if [ -f $incdir/gmp.h ]; then
        system_gmp_h=$incdir/gmp.h
    fi
done
if [ -n "$system_gmp_h" ]; then
    system_gmp_cc=`sed -n -e "$gmp_cc_pat" $system_gmp_h`
    system_gmp_cflags=`sed -n -e "$gmp_cflags_pat" $system_gmp_h`
fi

...

    else
        # 'native' not supported, see if GMP / MPIR provides us some CPU type:
        for opt in $gmp_cflags; do
            case $opt in
                -march=*|-mcpu=*|-mtune*)
                    echo "Found CPU parameter in gmp.h: $opt"
                    cpu_params="$cpu_params $opt"
                    ;;
                 # perhaps add other options, too (e.g. for different compilers)
                 *) other_gmp_cflags="$other_gmp_cflags $opt"
            esac
        done
    fi
    # Only add them if CFLAGS do not already contain similar:
    if [ -n "$cpu_params" ] &&
        ! (echo "$CFLAGS" | egrep -- '-march=|-mcpu=|-mtune=' >/dev/null);
    then
        echo "Using additional host-specific CFLAGS: $cpu_params"
        CFLAGS="$cpu_params $CFLAGS"
    fi
    if [ -n "$other_gmp_cflags" ]; then
        echo "Not using other CFLAGS provided by gmp.h: $other_gmp_cflags"
    fi
...

Putting at least the necessary flags into libm4ri.pc is of course another, non-exclusive option, and even on systems that lack pkg-config, you could use the .pc file by adding a few lines of shell, Python or whatever code.

You can also test whether adding -msse2 (if using SSE2 is desired) is necessary at all, or supported (for simplicity, here gcc-specific):

# Test whether '-msse2' is supported at all:
if $CC -msse2 -c -x c /dev/null -o /dev/null &>/dev/null; then
    # Ok, supported. Do we need it?
    # [Should perhaps check whether __SSE2__ is really non-zero.]
    if ($CC -E -dM -x c /dev/null | grep -w __SSE2__) &>/dev/null; then
        # Fine, we don't need to pass any options to enable SSE2.
        required_cflag=""
    else
        required_cflag="-msse2"
    fi
    # Check whether the flag conflicts with others, perhaps specified by the user:
    if $CC $CFLAGS $required_cflag -c -x c /dev/null -o /dev/null &>/dev/null; then
        # Ok, compatible and sufficient.
    else
        # Conflicting flags, issue error message or try something else...
    fi
else
    # Not supported, perhaps disable use of SSE2.
fi

(The same tests can easily be performed from Python as well, e.g. in [Sage's] setup.py or module_list.py. You could also steal macros from autotools.)

comment:50 follow-up: Changed 8 years ago by malb

The attached patch manually adds -msse -msse2 in module_list.py if __M4RI_HAVE_SSE2. I fully agree that M4RI should export its flags both as an .rc file and as a define like GMP and I will implement this as well.

comment:51 Changed 8 years ago by malb

Leif, note that M4RI checks for -msse2 like this

https://bitbucket.org/malb/m4ri/src/58007c8c739a/m4/ax_ext.m4

comment:52 Changed 8 years ago by leif

Replying to AlexanderDreyer:

Replying to malb:

The only caveat I could see with that approach is that it limits us to use !GCC. For Sage this isn't really a problem. M4RI supports more compilers. How's that for PolyBoRi?

We support only gcc yet. In the case that we would add additional ones we'll add a mapping of non-default compiler options anyway.

Yep. Adding such is almost trivial, at least for a fixed set of compilers (and platforms).


(e.g. -msse2 -> -xarch-sse2 for sun studio). But if you could export the original parameters, this would of course be better.

I'd do both, and resort to the SCons method in case no (suitable) parameters are available.


If we want to support more compilers I was thinking about dumping the CFLAGS to disk in M4RI which third party tools then can re-use. I was thinking of using the pkg-config.rc file which one can then either use via pkg-config or parse oneself.

I would prefer if you could add a macro defining the options as a string literal to m4ri_config.h. But if you need to setup the rc file anyway, you do not have to duplicate work.

I'd do both, but preferably use the header method. Using the .pc file is limited and to some extent dangerous, since pkg-config files lack (by default) a compiler command entry such that the flags may be invalid or insufficient for the actual compiler used.
(Note that some people even put flags into CC rather than CFLAGS, and different installed GCC versions might default to different architectures as well.)

You could either define other variables there, to get parsed / interpreted "manually", or simply put exactly the same lines you put into your header file into the .pc file, since incidentally any #define in a .pc file happens to be just a comment. :-)

(You'll have to use e.g. grep to extract them though, rather than pkg-config.)

comment:53 in reply to: ↑ 50 ; follow-up: Changed 8 years ago by leif

Replying to malb:

The attached patch manually adds -msse -msse2 in module_list.py if __M4RI_HAVE_SSE2. I fully agree that M4RI should export its flags both as an .rc file and as a define like GMP and I will implement this as well.

Shouldn't the modules also depend on SAGE_LOCAL + "/include/m4ri/m4ri_config.h"? (You should use SAGE_INC there, too, btw. ;-) )

This should be done in a more general or automatic way, i.e., m4ri_extra_compile_args (and probably -std=c99) should be added in setup.py for every module that links against libm4ri.

(I'm adding similar for dependencies, e.g. at #8664, and other stuff, in order to make module_list.py more modular and less error-prone.)

P.S.: SSE2 is a superset of SSE, just as -msse2 implies -msse.

The patch doesn't yet check whether adding these flags is sufficient. Haven't looked at your M4RI code, but if you add something like

#if defined(__M4RI_HAVE_SSE2) && __M4RI_HAVE_SSE2
#   if !defined(__SSE2__) || !__SSE2__
#       error Your current compiler and / or CFLAGS setting doesn't allow SSE2 code. Please change that or these to the setting(s) you used when compiling M4RI.
#   endif
#endif

(I wanted to post that to sage-devel a few hours ago, in response to:

> > If 'configure' determined the machine supports SSE2, __M4RI_HAVE_SSE2 
> > is set to 1, but even if that's the case, you should still also test 
> > __SSE2__ when compiling code later (including code *using* the 
> > library). 

> Why? I want that whoever links against M4RI lives in the same world as M4RI. 

for the reasons given above.)

comment:54 in reply to: ↑ 53 ; follow-up: Changed 8 years ago by leif

Replying to leif:

This should be done in a more general or automatic way, i.e., m4ri_extra_compile_args (and probably -std=c99) should be added in setup.py for every module that links against libm4ri.

P.S.: I don't mean you have to do / change that here, as e.g. Francois is also messing around in that area of setup.py ;-) , such that patches were likely to conflict, so I could also change that later on another ticket, perhaps unrelated to M4RI, but dealing with setup.py.

comment:55 Changed 8 years ago by AlexanderDreyer

A new PolyBoRi? 0.7.1 spkg (with activated -msse2 option, when necessary) can be found here: http://sage.math.washington.edu/home/dreyer/spkg/polybori-0.7.1.p5.spkg

comment:56 in reply to: ↑ 54 ; follow-up: Changed 8 years ago by fbissey

Replying to leif:

Replying to leif:

This should be done in a more general or automatic way, i.e., m4ri_extra_compile_args (and probably -std=c99) should be added in setup.py for every module that links against libm4ri.

P.S.: I don't mean you have to do / change that here, as e.g. Francois is also messing around in that area of setup.py ;-) , such that patches were likely to conflict, so I could also change that later on another ticket, perhaps unrelated to M4RI, but dealing with setup.py.

All my changes have been merged in 4.7.2.alpha1 I don't see them getting out again. So I would say it is safe to base your patch on that.

comment:57 in reply to: ↑ 56 Changed 8 years ago by leif

Replying to fbissey:

Replying to leif:

P.S.: I don't mean you have to do / change that here, as e.g. Francois is also messing around in that area of setup.py ;-) , such that patches were likely to conflict, so I could also change that later on another ticket, perhaps unrelated to M4RI, but dealing with setup.py.

All my changes have been merged in 4.7.2.alpha1 I don't see them getting out again. So I would say it is safe to base your patch on that.

But I've to again rebase my patch from #8664 on yours... :D

P.S.: Some good and bad news for #9958 coming soon...

comment:58 Changed 8 years ago by fbissey

Indeed, I mothballed SAGE_DEBIAN ! I didn't notice you had a patch touching that area in #8664, it took too long to merge that one really. It has to go in.

I am bracing myself for #9958, you know we already ship sage-on-gentoo with python-2.7 and it works well (#11339 excluded) on x86/amd64 linux and OS X in a prefix. But we are getting off-track.

comment:59 Changed 8 years ago by AlexanderDreyer

The PolyBoRi spkg has now its own ticket: #11756 It it solved the problem (it does for me on a Gentoo VM), please review.

comment:60 follow-up: Changed 8 years ago by malb

Hi,

so how's this for a strategy:

  • we do the fixes (or something similar) that Alexander and I proposed for PolyBoRi and !Sage respectively now
  • I provide (in a new ticket) an updated M4RI which exports its !CLFAGS both in the headers and in the .rc file and also checks for SSE2 to fail gracefully.
  • We then update !Sage & PolyBoRi again to use these !CFLAGS?

comment:61 Changed 8 years ago by strogdon

polybori-0.7.1.p5.spkg does build. I applied the two indicated patches but building of sage itself fails:

gcc -fno-strict-aliasing -g -O2 -DNDEBUG -g -O3 -Wall -Wstrict-prototypes -fPIC -I/storage/sage-python2.6/sage-4.7.2.alpha1/local/include -I/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/csage -I/storage/sage-python2.6/sage-4.7
.2.alpha1/devel/sage/sage/ext -I/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/python2.6 -c sage/matrix/matrix_integer_dense.c -o build/temp.linux-i686-2.6/sage/matrix/matrix_integer_dense.o -std=c99 -w
In file included from /storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:42:0,
                 from /storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/permutation.h:31,
                 from /storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/m4ri.h:50,
                 from sage/matrix/matrix_integer_dense.c:240:
/storage/strogdon/gentoo/usr/lib/gcc/i686-pc-linux-gnu/4.5.2/include/emmintrin.h:32:3: error: #error "SSE2 instruction set not enabled"
In file included from /storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/permutation.h:31:0,
                 from /storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/m4ri.h:50,
                 from sage/matrix/matrix_integer_dense.c:240:
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h: In function mzd_row_add_offset:
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:741:5: error: __m128i undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:741:5: note: each undeclared identifier is reported only once for each function it appears in
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:741:14: error: __src undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:741:31: error: expected expression before ) token
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:742:14: error: __dst undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:742:31: error: expected expression before ) token
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:743:14: error: expected expression before const
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:746:15: error: expected ; before xmm1
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:747:18: error: xmm1 undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:749:21: error: eof undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h: In function mzd_combine_even_in_place:
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1190:7: error: __m128i undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1190:16: error: a128 undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1190:32: error: expected expression before ) token
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1191:16: error: b128 undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1191:32: error: expected expression before ) token
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1192:21: error: expected =, ,, ;, asm or __attribute__ before * token
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1192:22: error: eof undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1192:37: error: expected expression before ) token
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h: In function mzd_combine_even:
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1274:7: error: __m128i undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1274:16: error: a128 undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1274:32: error: expected expression before ) token
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1275:16: error: b128 undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1275:32: error: expected expression before ) token
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1276:16: error: c128 undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1276:32: error: expected expression before ) token
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1277:21: error: expected =, ,, ;, asm or __attribute__ before * token
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1277:22: error: eof undeclared (first use in this function)
/storage/sage-python2.6/sage-4.7.2.alpha1/local/include/m4ri/packedmatrix.h:1277:37: error: expected expression before ) token

comment:62 Changed 8 years ago by malb

Okay, I know what the issue is and will update the patch accordingly!

comment:63 in reply to: ↑ 60 Changed 8 years ago by leif

Replying to malb:

so how's this for a strategy:

  • we do the fixes (or something similar) that Alexander and I proposed for PolyBoRi and !Sage respectively now
  • I provide (in a new ticket) an updated M4RI which exports its !CLFAGS both in the headers and in the .rc file and also checks for SSE2 to fail gracefully.
  • We then update !Sage & PolyBoRi again to use these !CFLAGS?

!That ?sounds !reasonable!!

And also avoids circular ticket dependencies for the follow-up(s)...

comment:64 Changed 8 years ago by leif

... though you could easily add the graceful dead here' as well.

comment:65 Changed 8 years ago by malb

I wanted to add it to M4RI and so far the update to this ticket does not require a new M4RI SPKG. I'm in the process of patching M4RI and will open a new ticket for it shortly. Once that's done we can easily make that a dependency for this ticket?

Changed 8 years ago by malb

comment:66 follow-up: Changed 8 years ago by malb

  • Status changed from needs_work to needs_review

The updated patch should allow the Sage build to go through. Testing on cicero now. I also added some depends fields as suggested by Leif.

comment:67 follow-up: Changed 8 years ago by malb

The new M4RI SPKG is at #11757

comment:68 in reply to: ↑ 67 ; follow-up: Changed 8 years ago by SimonKing

  • Status changed from needs_review to needs_info

Replying to malb:

The new M4RI SPKG is at #11757

Does that mean that #11757 makes this ticket a duplicate?

comment:69 in reply to: ↑ 66 Changed 8 years ago by strogdon

Replying to malb:

The updated patch should allow the Sage build to go through. Testing on cicero now. I also added some depends fields as suggested by Leif.

Yes, the updated patch does allow Sage to build and there are no failing tests.

comment:70 in reply to: ↑ 68 Changed 8 years ago by malb

Replying to SimonKing:

Replying to malb:

The new M4RI SPKG is at #11757

Does that mean that #11757 makes this ticket a duplicate?

I guess one could look at it this way. I'd suggest: We deal with this ticket here first and if people then still have energy and are not sick of M4RI yet we can also include #11757.

comment:71 Changed 8 years ago by leif

  • Status changed from needs_info to needs_review

comment:72 follow-up: Changed 8 years ago by malb

So, anybody up for reviewing this? Or are people waiting for trac_11574_m4ri_sse2.patch being ported to #11757?

comment:73 in reply to: ↑ 72 ; follow-ups: Changed 8 years ago by strogdon

Replying to malb:

So, anybody up for reviewing this? Or are people waiting for trac_11574_m4ri_sse2.patch being ported to #11757?

I should probably defer review to Simon since I got in through the backdoor. However, since Sage builds here and there are no doctest failures I would give it a positive review. I do have one question. Shouldn't this ticket depend upon ticket #11756, even though that one has a positive review?

comment:74 in reply to: ↑ 73 Changed 8 years ago by leif

  • Dependencies changed from #11261 to #11261 #11756

Replying to strogdon:

Shouldn't this ticket depend upon ticket #11756, even though that one has a positive review?

Yep, IMHO even if #11756 had already been merged into some devel release.

comment:75 in reply to: ↑ 73 Changed 8 years ago by SimonKing

Replying to strogdon:

Replying to malb:

So, anybody up for reviewing this? Or are people waiting for trac_11574_m4ri_sse2.patch being ported to #11757?

I should probably defer review to Simon since I got in through the backdoor.

Sorry, I will probably not be able to review it in the next couple of days. So, iI'd appreciate if someone was quicker than I.

comment:76 Changed 8 years ago by malb

  • Description modified (diff)

anyone?

comment:77 Changed 8 years ago by AlexanderDreyer

  • Cc AlexanderDreyer added
  • Reviewers changed from Simon King to Simon King, Alexander Dreyer
  • Status changed from needs_review to positive_review

The spkg builds and and installs fine. Together with the patches sage -testall was run successfully on a SuSE Linux Enterprise 11 x86_64, Gentoo i486, and Mac OS X 10.5 ppc.

So, I can cordially give a positive review!

comment:78 Changed 8 years ago by leif

m4ri_20110601.patch has no ticket number in its commit message.

Changed 8 years ago by malb

rebased to 4.7.1.alpha4

comment:79 Changed 8 years ago by malb

fixed.

comment:80 Changed 8 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.