Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#14754 closed enhancement (fixed)

Update ATLAS to stable version 3.10.1

Reported by: jdemeyer Owned by: jdemeyer
Priority: critical Milestone: sage-5.12
Component: packages: standard Keywords:
Cc: Merged in: sage-5.12.beta0
Authors: Volker Braun, Jeroen Demeyer, Jean-Pierre Flori Reviewers: Benjamin Jones, Karl-Dieter Crisman, Dmitrii Pasechnik, Georg Weber, François Bissey, John Palmieri, Volker Braun, Jean-Pierre Flori
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jpflori)

  • (see also #14781): move untracked files to src/ in preparation for git
  • Added spkg-src
  • Remove the archdef_dir variable, copy our own archdefs into src/ instead
  • Match isa_ext level to existing archdefs in order to build atlas faster in the fast/base targets

Spkg diffs:

Updated spkg (based on #10508 and #14699):

  1. http://boxen.math.washington.edu/home/vbraun/spkg/atlas-3.10.1.p3.spkg

Apply (based on #10508):

  1. 14754_root.patch to the SAGE_ROOT repository
  2. 14754_update_atlas_docs.patch to the Sage repository.
  3. trac_14754-new_opts.patch to the Sage repository.

Remove the lapack and blas packages.

Attachments (6)

14754_root.patch (3.6 KB) - added by jdemeyer 8 years ago.
14754_update_atlas_docs.patch (4.6 KB) - added by jdemeyer 8 years ago.
atlas-3.10.1.p2.diff (4.0 KB) - added by jdemeyer 8 years ago.
spkg diff
test_atlas.sh (882 bytes) - added by vbraun 8 years ago.
Script to build atlas with base, fast, and default settings
atlas-p2-p3.diff (25.8 KB) - added by vbraun 8 years ago.
for review only
trac_14754-new_opts.patch (1.3 KB) - added by jpflori 8 years ago.
Document new options.

Download all attachments as: .zip

Change History (61)

comment:1 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:4 Changed 8 years ago by jdemeyer

When removing 3DNow, it starts using SSE1 and SSE2, which shouldn't happen either. But this also happens with the old ATLAS spkg, so at least it's not a regression.

Last edited 8 years ago by jdemeyer (previous) (diff)

comment:5 follow-up: Changed 8 years ago by vbraun

You should use isa_ext=('None',) if you want to explictly disable it, see ATLAS_ISAEXT.

The given extensions are not mandatory but are supposed to let ATLAS try them. Whether or not they end up being used is up to the ATLAS build system and how well they perform. Though it seems that you found a bug with ancient cpus, which is unsurprising since I'm pretty sure that nobody else than us builds ATLAS on museum pieces.

comment:6 in reply to: ↑ 5 Changed 8 years ago by jdemeyer

Replying to vbraun:

You should use isa_ext=('None',) if you want to explictly disable it, see ATLAS_ISAEXT.

Done in linked spkg, not yet tested.

comment:7 Changed 8 years ago by jdemeyer

  • Description modified (diff)

Changed 8 years ago by jdemeyer

comment:8 Changed 8 years ago by jdemeyer

  • Description modified (diff)

Changed 8 years ago by jdemeyer

comment:9 Changed 8 years ago by jdemeyer

  • Description modified (diff)

Changed 8 years ago by jdemeyer

spkg diff

comment:10 Changed 8 years ago by jdemeyer

  • Status changed from new to needs_review

comment:11 Changed 8 years ago by vbraun

On 64 bit intel chips we should definitely enable SSE2

Edit: misread the patch, the SSE2 is there but there is no 64-bit x86SSE264 archdef. But I guess thats ok, we wil just run through the tuning. There is no 32-bit archdef for ancient 64-bit intel chips

Last edited 8 years ago by vbraun (previous) (diff)

comment:12 Changed 8 years ago by vbraun

And the real question: does it improve the build time on ancient i386 chips without SSE? The only real improvement would be if the pure x87 archdef would be used and is actually faster to compile than the full tuning.

comment:13 Changed 8 years ago by jdemeyer

Volker, you obviously understand this stuff better than me, so if you want to make a reviewer patch, go ahead.

My main concern is that configure_base() should produce a generic binary, meaning it runs on all processors of a given architecture.

(although note that the oldest i386-architecture processor that we test Sage on is a Pentium 4, which has SSE and SSE2)

comment:14 follow-up: Changed 8 years ago by vbraun

There is no way to make a binary that will run on all x86 cpus ever made. For starters, the original x86 didn't even have x87. Its an evolving instruction set, and we should aim at "everything within the last X years" and not the most dumb binary that we can possibly build. Otherwise we'll just make false claims since nobody is going to be able to test that the binary actually works.

The only reason for removing the 3DNow would be if that makes build time faster since the naming scheme for the "generic i386 archdef" is without it. Which is why I'm asking if it actually helps. And for that I guess we'll have to time builds on cicero, maybe I'll get around to it after the 22nd when you are gone.

comment:15 in reply to: ↑ 14 Changed 8 years ago by jdemeyer

Replying to vbraun:

The only reason for removing the 3DNow

As far as I know, 3DNow is AMD-only, so you would be excluding all Intel chips...

comment:16 Changed 8 years ago by jpflori

Indeed, that's what i found as well, so enabling 3DNow was a bad idea. I suggest to use:

  • the x86x87 target with no ISA extension on 32 bits, it should cover most CPUs except for the ones which have been even discarded from museum,
  • x86 plus SSE2 for 64 bits.

Note ATLAS provide archdefs for the former and Volker added archdefs for the latter.

comment:17 Changed 8 years ago by jpflori

So basically I completely agree with what Jeroen did.

How are the generic and non-generic build going on?

comment:18 Changed 8 years ago by jpflori

(About the x86x87 target, ATLAS doc at http://math-atlas.sourceforge.net/atlas_install/node28.html states:

(I believe this is all Intel architectures from PentiumPRO onwards, and any AMD platform since at least the original Athlon)

I would feel this is quite old enough.

comment:19 follow-up: Changed 8 years ago by vbraun

I agree that the change is correct if it actually uses the archdef now. So there is some testing to be done.

In the absence of archdefs we should still enable 3Dnow (and SSE in a few years). x86x87 is already an isa extension over stone-age x86. And "enabling" means: try to build the kernel. Whether or not it is actually used by ATLAS depends on whether it actually compiles, runs, and performs well. Not that I think that there is any particular 3dnow asm in ATLAS, but in the near future this will be important for SSE.

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

Replying to vbraun:

we should still enable 3Dnow (and SSE in a few years).

That makes no sense at all. Intel Pentium 4 (quite old but not stone-age) has SSE and SSE2 but not 3DNow.

Not that I think that there is any particular 3dnow asm in ATLAS, but in the near future this will be important for SSE.

There surely is, otherwise I wouldn't be seeing all those "Illegal Instruction" errors.

Last edited 8 years ago by jdemeyer (previous) (diff)

comment:21 follow-up: Changed 8 years ago by jpflori

  • Status changed from needs_review to needs_work
  • Work issues set to archdef_dir, PATCH_DIR

In spkg-install, the "archdef_dir = PATCH_DIR" line for the base target on x86 64 bits should be removed. I've moved the tarballs into a ARCH subdir and reenabled copying of archdefs tarballs into ATLAS proper dir, so it should find them without tweaking archdef_dir. Or you could disable the copying and set archdef_dir...

By the way, for the fast target we could also refactor the archdef_dir setting and isa_ext, potentially setting isa_ext = ('None',) by default as for the base target.

Question for Volker, how to tell for sure that archdefs are used? apart from a shorter building time?

Apart from this, should we wait for the patchbots reports before putting this to positive_review? Or the other way around?

comment:22 in reply to: ↑ 20 ; follow-up: Changed 8 years ago by vbraun

Replying to jdemeyer:

There surely is, otherwise I wouldn't be seeing all those "Illegal Instruction" errors.

I don't think so. Unless the build host supports 3dnow there is no way that such a kernel would end up in the atlas library. The illegal instructions are because the previous atlas version (which we are still shipping) erroneously did not respect the given limits to the ISA.

comment:23 in reply to: ↑ 21 Changed 8 years ago by vbraun

Replying to jpflori:

Question for Volker, how to tell for sure that archdefs are used? apart from a shorter building time?

You can probably read the log, but the only way I would really trust is to measure the build time.

comment:24 in reply to: ↑ 22 Changed 8 years ago by jdemeyer

Replying to vbraun:

I don't think so. Unless the build host supports 3dnow there is no way that such a kernel would end up in the atlas library. The illegal instructions are because the previous atlas version (which we are still shipping) erroneously did not respect the given limits to the ISA.

You can think what you want, but the fact is that the build of ATLAS-3.10.1 on cicero failed with 3DNow (with Illegal Instruction messages in the build log) but succeeded without 3DNow (without Illegal Instruction messages in the build log).

Last edited 8 years ago by jdemeyer (previous) (diff)

comment:25 Changed 8 years ago by vbraun

For the record, I built ATLAS with

SAGE_ATLAS_ARCH=x86x87,3DNow

and I do get a working install which passes the Sage doctests. So, just to spell it out again, the ISA extension settings just tell ATLAS which ones it is allowed to try, not which ones end up in the ATLAS library. Its normal to get SIGILL during the atlas tuning.

I also verified that it goes through a full tuning which takes a very long time. Presumably that just doesn't work on cicero, or when you tried it the build system couldn't get stable timings.

comment:26 Changed 8 years ago by vbraun

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues archdef_dir, PATCH_DIR deleted

comment:27 Changed 8 years ago by vbraun

Both the fast and base targets for 32-bit intel did not make use of the existing archdefs because of the isa extension mismatch in the filename. Fixed now.

comment:28 follow-up: Changed 8 years ago by jpflori

Great that you provided a proper spkg-src script. I'll try to review the spkg tomorrow.

And about 3DNow (I saw you did not put it in back), we don't want to put it in base or fast anyway because it's AMD only, so if you happen to build a dist tarball of Sage, typically by setting the FAT option, on a computer with 3DNow, it will get in and later segfault if you run it on an Intel computer.

comment:29 in reply to: ↑ 28 ; follow-up: Changed 8 years ago by vbraun

Replying to jpflori:

And about 3DNow, we don't want to put it in base or fast anyway because it's AMD only

Kind of a moot point since we don't distribute binaries built on such an old machine. Also, SAGE_FAT_BINARY is "base" so the 3DNow would be fair game for the "fast" target. Except that, there, we already require SSE2.

comment:30 Changed 8 years ago by vbraun

When I build the 32-bit base/fast versions in my 32-bit virtual machine then I get doctest errors in Sage. Might be something wrong with my VM, though.

comment:31 follow-up: Changed 8 years ago by vbraun

The doctest failures go away if I disable threading, so perhaps this breaks because I'm building on an quad-core i7. Unless sombody objects, I'd say we disable threading on 32-bit intel base/fast targets. This should probably be done independent of this issue because the "generic" library shouldn't have multithreaded thresholds being derived from the latest CPUs with lots of cores.

comment:32 Changed 8 years ago by jpflori

I don't really like the idea... Please also see #14605, the way threading is dealt with is kindof completely broken right now.

comment:33 follow-up: Changed 8 years ago by vbraun

Tell me about it.. I took your threads.patch to make it build at all.

But IMHO its much more important to get an ATLAS update in Sage than fixing threading bugs. So if that means we have to disable it on 32-bit then that sucks but is still better than the current state.

comment:34 follow-up: Changed 8 years ago by vbraun

PS: The updated spkg makes the thread limit configurable via, for example,

SAGE_ATLAS_ARCH=threads=2,fast

Setting the limit to 0 disables threading. Specifying the thread limit is completely optional. This is useful for testing.

comment:35 Changed 8 years ago by vbraun

In fact, the threaded ATLAS doesn't work on x86_64 either. I must have run the doctests in the wrong Sage install the other day?

comment:36 Changed 8 years ago by vbraun

My bad, liblinbox was linked with the system atlas (version 3.8.4). Mixing its threading symbols with ATLAS-3.10.1 is instant disaster.

comment:37 in reply to: ↑ 29 Changed 8 years ago by jpflori

Replying to vbraun:

Replying to jpflori:

And about 3DNow, we don't want to put it in base or fast anyway because it's AMD only

Kind of a moot point since we don't distribute binaries built on such an old machine. Also, SAGE_FAT_BINARY is "base" so the 3DNow would be fair game for the "fast" target. Except that, there, we already require SSE2.

It does not tackle the problem that 3DNow is AMD only... I could imagine someone wanting to distribute a sage bdist with a "fast" but still somehow portable ATLAS. You can still argue that in that case he can very well remove 3DNow himself if he wants ot play with fire...

I just don't like to add non-universal extensions to predifined targets although I completely agree that if we consider that someone who builds with fast should only expect it to work on the machine where it was built then trying 3DNow makes sense.

Anyway, does using 3DNow in addition of SSE or SSE2 gives any speedup? I guess not, so let's forget about 3DNow for "fast" because we have SSE2 and for "base" because we don't want people to end up distributing AMD only binaries (though it's highly unlikely, I agree)...

comment:38 in reply to: ↑ 34 Changed 8 years ago by jpflori

Replying to vbraun:

PS: The updated spkg makes the thread limit configurable via, for example,

SAGE_ATLAS_ARCH=threads=2,fast

Setting the limit to 0 disables threading. Specifying the thread limit is completely optional. This is useful for testing.

Great.

comment:39 in reply to: ↑ 33 Changed 8 years ago by jpflori

Replying to vbraun:

Tell me about it.. I took your threads.patch to make it build at all.

But IMHO its much more important to get an ATLAS update in Sage than fixing threading bugs. So if that means we have to disable it on 32-bit then that sucks but is still better than the current state.

Is this really because of the VM (or the OS?) being 32 bits? It seems strange. Or did you also get mixed symbols?

Anyway, I agree the base target should disable threading. For the fast one, if we go for "the "fast" target should only run on the machine where it is built" interpretation, then it's kind of bad. But if threading is indeed broken on all 32 bits OSes, then we don't have a choice. If we want to consider that the "fast" build is also "generic" and could be redistributed, then disabling threading is completely ok, till we decide that recent enough computers have more than one core :)

comment:40 in reply to: ↑ 31 Changed 8 years ago by jpflori

Replying to vbraun:

This should probably be done independent of this issue because the "generic" library shouldn't have multithreaded thresholds being derived from the latest CPUs with lots of cores.

You mean reenabling threading on 32 bits intel? (In case it was not broken just because of mixed symbols of course.)

comment:41 Changed 8 years ago by vbraun

System ATLAS is (at least on Fedora) libatlas.so.3 and we install libatlas.so.2. Since I rebuilt various packages mixing system atlas (using SAGE_ATLAS_LIB) and self-compiled atlas, I ended up with a mix of symbols. I've changed our versioning to use .3 as well. Upstream doesn't version the library but since it is ATLAS 3.x most distributors will probably opt for .3.

I agree that there is no point in 3DNow on SSE-enabled processors. Its not used anywhere now.

I think that was the problem with 32-bit, too. But I'm still running tests. Will re-enable "fast" threads on 32-bit if it works...

I've changed the threads option to colon

SAGE_ATLAS_ARCH=threads:2,static,fast

and added the optional "static" to also install static libraries (though built with -fPIC, really only useful for debugging)

Finally, I'm also installing upstream's new ATLAS shared library (libsatlas.so / libtatlas.so). Eventually we'll want to switch Sage over to those, so we might just as well install them already.

comment:42 Changed 8 years ago by jpflori

Great only good news. I also kind of disliked the multiple equal signs to control threading.

comment:43 Changed 8 years ago by jpflori

Note that having a static atlas is useful for Cygwin as libtool won't build the shared library there.

comment:44 Changed 8 years ago by jpflori

In particular, I suggest that if static is enabled, then don't let the whole spkg-install script fail if building the shared library failed, but building (and installing) the static one did not.

comment:45 follow-up: Changed 8 years ago by vbraun

Does the Cygwin build actually get that far? ATLAS configure has a bunch of Cygwin-related settings and we don't touch any of that stuff.

comment:46 Changed 8 years ago by vbraun

I do get the error reported on #14753

sage -t --long sage/matrix/matrix_double_dense.pyx
**********************************************************************
File "sage/matrix/matrix_double_dense.pyx", line 3809, in sage.matrix.matrix_double_dense.Matrix_double_dense.is_positive_definite
Failed example:
    H.is_positive_definite()
Expected:
    True
Got:
    False
**********************************************************************

also with Fedora's version of ATLAS-3.8.4. I'm not sure that the test is actually numerically well-conditioned, it does:

sage: R = random_matrix(CDF, 200)
sage: H = R.conjugate_transpose()*R
sage: import scipy.linalg
sage: scipy.linalg.cholesky(H.numpy(), lower=1)

Depending on the distribution of random_matrix this may be ill-conditioned: http://stackoverflow.com/questions/12322431/generating-a-pseduo-random-positive-definite-matrix. Conditioning numbers are around 10^6 - 10^7 so its not great but not terribly bad either.

Changed 8 years ago by vbraun

Script to build atlas with base, fast, and default settings

comment:47 in reply to: ↑ 45 Changed 8 years ago by jpflori

Replying to vbraun:

Does the Cygwin build actually get that far? ATLAS configure has a bunch of Cygwin-related settings and we don't touch any of that stuff.

Last time I tried it finished.

I could even build shared libraries by replacing libtool calls to gcc calls for linking.

Changed 8 years ago by vbraun

for review only

comment:48 Changed 8 years ago by vbraun

I built base, fast, and the default (no SAGE_ATLAS_ARCH) on a variety of machines and make ptestlong succeeds in all cases. Wall time to build ATLAS:

                  |    base     |     fast    |    default
------------------+-------------+-------------+--------------
My Thinkpad W530  |  20m52.259s |  14m34.608s |  13m52.420s
Fedora 19 x86_64  |             |             |
------------------+-------------+-------------+--------------
Virtual Machine   |  22m16.444s |  22m23.433s | 244m25.015s
Fedora 19 i686    |             |             |
------------------+-------------+-------------+--------------
cicero on skynet  |  38m49.345s |  38m50.003s |  38m24.546s
                  |             |             |

comment:49 Changed 8 years ago by jpflori

The new patches introduce a typo:

raise ValuError

I guess you could also get rid of "autom4te.cache" in spkg-src.

Maybe document the new options threads and static.

Otherwise everything looks fine, I'm testing it right now.

comment:50 Changed 8 years ago by vbraun

Updated spkg, diff is

diff -r 4c3c4795e2b6 -r d32570dc368b spkg-install
--- a/spkg-install	Sun Jul 07 01:00:12 2013 -0400
+++ b/spkg-install	Tue Jul 16 16:13:17 2013 -0400
@@ -232,7 +232,7 @@
             INSTALL_STATIC_LIBRARIES = True
         elif opt in ATLAS_MACHTYPE + ('fast', 'base'):
             if arch is not None:
-                raise ValuError('multiple architectures specified: {0} and {1}'.format(arch, opt))
+                raise ValueError('multiple architectures specified: {0} and {1}'.format(arch, opt))
             arch = opt
         elif opt in ATLAS_ISAEXT:
             if isa_ext is None:
diff -r 4c3c4795e2b6 -r d32570dc368b spkg-src
--- a/spkg-src	Sun Jul 07 01:00:12 2013 -0400
+++ b/spkg-src	Tue Jul 16 16:13:17 2013 -0400
@@ -49,6 +49,7 @@
 cp -rp "$SPKG_ROOT"/patches/ATLAS-lib .
 cd ATLAS-lib
 autoreconf -fiv
+rm -rf autom4te.cache
 
 
 ### Finished creating the src/ directory

Changed 8 years ago by jpflori

Document new options.

comment:51 Changed 8 years ago by jpflori

  • Description modified (diff)
  • Reviewers changed from Benjamin Jones, Karl-Dieter Crisman, Dmitrii Pasechnik, Georg Weber, François Bissey, John Palmieri, Volker Braun to Benjamin Jones, Karl-Dieter Crisman, Dmitrii Pasechnik, Georg Weber, François Bissey, John Palmieri, Volker Braun, Jean-Pierre Flori

I'm happy with Volker changes.

If he agrees with my changes to the doc, let's put this as positive review.

comment:52 Changed 8 years ago by vbraun

  • Status changed from needs_review to positive_review

Thanks, looks good to me.

comment:53 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:54 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.12.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:55 Changed 8 years ago by jdemeyer

See #15045 for a blocker follow-up.

Note: See TracTickets for help on using tickets.