Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

atlas not respecting SAGE_FAT_BINARY on i686 systems #9382

Closed
sagetrac-mariah mannequin opened this issue Jun 29, 2010 · 27 comments
Closed

atlas not respecting SAGE_FAT_BINARY on i686 systems #9382

sagetrac-mariah mannequin opened this issue Jun 29, 2010 · 27 comments

Comments

@sagetrac-mariah
Copy link
Mannequin

sagetrac-mariah mannequin commented Jun 29, 2010

atlas on i686 systems (x86-Linux) is
not respecting SAGE_FAT_BINARY.

The attached mercurial patch adds
i686 to the list of other architectures
(i386, x86_64) that respect SAGE_FAT_BINARY.

Component: packages: standard

Reviewer: Leif Leonhardy, Volker Braun

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

@sagetrac-mariah sagetrac-mariah mannequin added this to the sage-4.7.1 milestone Jun 29, 2010
@sagetrac-mariah
Copy link
Mannequin Author

sagetrac-mariah mannequin commented Jun 29, 2010

comment:1

Attachment: atlas-i686-FAT.patch.gz

@williamstein
Copy link
Contributor

comment:2

Very likely positive review -- just need Robert Miller to try it on some 32-bit linux box.

@rlmill
Copy link
Mannequin

rlmill mannequin commented Jul 9, 2010

comment:3

I get an error installing atlas with this patch:

Host system
uname -a:
Linux cicero 2.6.32.12-115.fc12.i686.PAE #1 SMP Fri Apr 30 20:14:08 UTC 2010 i686 i686 i386 GNU/Linux
****************************************************
****************************************************
CC Version
gcc -v
Using built-in specs.
Target: i686-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-languages=c,c++,objc,obj-c++,java,fortran,ada --enable-java-awt=gtk --disable-dssi --enable-plugin --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre --enable-libgcj-multifile --enable-java-maintainer-mode --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libjava-multilib --with-ppl --with-cloog --with-tune=generic --with-arch=i686 --build=i686-redhat-linux
Thread model: posix
gcc version 4.4.4 20100630 (Red Hat 4.4.4-10) (GCC) 
****************************************************
Platform detected to be 32 bits
system_atlas.py:6: DeprecationWarning: os.popen2 is deprecated.  Use the subprocess module.
  fortran = os.popen2(os.environ['SAGE_LOCAL']+'/bin/'+'which_fortran')[1].read()
./spkg-install-script: line 119: syntax error near unexpected token `}'
./spkg-install-script: line 119: `}'
Failed to build ATLAS.

real    0m0.326s
user    0m0.129s
sys     0m0.122s
sage: An error occurred while installing atlas-3.8.3.p13

Looking at the patch, do I see a mismatched single quote?

@rlmill rlmill mannequin added s: needs work and removed s: needs review labels Jul 9, 2010
@jhpalmieri
Copy link
Member

comment:4

Looking at the patch, do I see a mismatched single quote?

That's what it looks like to me. What if you just delete the quote? Replace

if [ "`uname -p`" = "i386" -o "'`uname -p`" = "i686" -o `uname -p`" = "x86_64" ]; then 

with

if [ "`uname -p`" = "i386" -o "`uname -p`" = "i686" -o `uname -p`" = "x86_64" ]; then 

(no single quote before the second `uname...`).

@rlmill
Copy link
Mannequin

rlmill mannequin commented Jul 11, 2010

comment:5

The third uname then has a mismatched double quote.

@sagetrac-mariah
Copy link
Mannequin Author

sagetrac-mariah mannequin commented Jul 15, 2010

comment:6

Apologies for my silly error. Here is a revised patch 9382.patch and the corresponding spkg atlas-3.8.3.p13.spkg.

@dandrake
Copy link
Contributor

dandrake commented Nov 9, 2010

comment:7

A similar problem has been noticed in Arch Linux: https://bugs.archlinux.org/task/21592

18:44 < td123> hello
18:45 < td123> it seems that when building sage, it doesn't respect the 
               SAGE_FAT_BINARY='yes' option because I built sage with that 
               option on a corei7 for x86 and someone kept getting illegal 
               instruction on a pentium 3
18:45 < td123> when I looked through the logs, it seemed that atlas (among 
               possibly others) were building with processort specific 
               optimizations
18:48 < td123> I'm the packager for archlinux :) 
http://repos.archlinux.org/wsvn/community/sage-mathematics/trunk/PKGBUILD is 
               the package source

@wjp
Copy link
Mannequin

wjp mannequin commented Dec 6, 2010

comment:8

uname -p on linux returns a textual description of the CPU for me, not a canonical architecture name. It looks like this may have to be uname -m on Linux.

On the other hand, on solaris it looks like uname -p is correct.

As td123 on IRC suggested, maybe we should just check both the results of -m and -p for these values?

@wjp wjp mannequin added s: needs work and removed s: needs review labels Dec 6, 2010
@wjp
Copy link
Mannequin

wjp mannequin commented Jan 8, 2011

@wjp
Copy link
Mannequin

wjp mannequin commented Jan 8, 2011

Changed author from Mariah Lenox to Mariah Lenox, Willem Jan Palenstijn

@wjp wjp mannequin added s: needs review and removed s: needs work labels Jan 8, 2011
@vbraun
Copy link
Member

vbraun commented Jan 9, 2011

comment:10

Willem: which machine and which uname outputs are you referring to in comment 8?

Instead of hacking around the current atlas build script, I think it would be a good point to switch to my rewritten atlas spkg at #10226 that detects the configuration in a much cleaner way.

@wjp
Copy link
Mannequin

wjp mannequin commented Jan 9, 2011

comment:11

Nearly every linux machine I've tried, with a few exceptions where uname -m and uname -p give the same output.

Example (a 64 bit Gentoo machine):

mira: ~ > uname -m
x86_64
mira: ~ > uname -p
Intel(R) Xeon(R) CPU X3220 @ 2.40GHz

@dandrake
Copy link
Contributor

updated version of previous patches

@dandrake
Copy link
Contributor

comment:12

Attachment: 9382.patch.gz

Replying to @sagetrac-mariah:

Apologies for my silly error. Here is a revised patch 9382.patch

That patch has another tiny error: it has "uname-p" (no space between "uname" and "-p"). I just uploaded a patch to this ticket in which I fixed that problem. However, there are other problems: on Ubuntu, it seems that uname -p returns "unknown" and uname -m returns the architecture name that we want. I've tried this on several 32-bit and 64-bit (i.e., i686 and x86_64) computers, on Ubuntu 8.04, 10.04, and 10.10 and they all behave the same. So we will need to use "-m" to correctly detect this on Ubuntu.

@wjp
Copy link
Mannequin

wjp mannequin commented Jan 11, 2011

comment:14

You're looking at an old version of the patch. The most up to date one is the SPKG in comment #9.

Also, #10226 (which also needs review) might supersede this patch.

@wjp wjp mannequin added s: needs review and removed s: needs work labels Jan 11, 2011
@dandrake
Copy link
Contributor

comment:15

Replying to @wjp:

Also, #10226 (which also needs review) might supersede this patch.

It does, and I think we should work on that ticket and close this one when #10226 gets merged.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 3, 2011

comment:16

Replying to @dandrake:

Replying to @wjp:

Also, #10226 (which also needs review) might supersede this patch.

It does, and I think we should work on that ticket and close this one when #10226 gets merged.

I currently don't know whether the new (Python install script) ATLAS spkg from #10226 supports SAGE_FAT_BINARY as desired, but for the record:

  • uname -p isn't portable (it may return "unknown" even on Linuces), one should use uname -m instead.

  • Rather than having (btw. incomplete)

    if [ ... -o ... -o ... -o ... ]; then ...

one should use

    case "`uname -m`" in
      i[3456]86|i86pc)
        ...
        ;;
      amd64|x86_64)
        ...
        ;;
      ia64) # Itanium
        ...
        ;;
      # etc.
    esac

instead, which is not only more readable, but also more robust (and portable w.r.t. broken / buggy test commands).

@vbraun
Copy link
Member

vbraun commented Aug 3, 2011

comment:17

For the record, the updated ATLAS spkg from #10226 uses the $UNAME environment variable (which Sage sets) if available, and python's portable platform.system() otherwise:

try:
    conf['system'] = os.environ['UNAME']
except KeyError:
    conf['system'] = platform.system()

Also there is some support for SAGE_FAT_BINARY:

    if os.environ.get('SAGE_FAT_BINARY', 'no') == 'yes' and conf['Intel?']:
        print 'Sage "fat" binary mode set: Building SSE2 only Hammer binary'
        print 'NOTE: This can result in a Sage that is significantly slower at certain numerical'
        print 'linear algebra since full FAT binary support has not been implemented yet.'
        arch = 'HAMMER'
        isa_ext = ('SSE2', 'SSE1')

Though I'm not sure if anybody ever verified that ATLAS, with these settings, indeed does not use any more advanced isa extensions.

@vbraun
Copy link
Member

vbraun commented Aug 3, 2011

comment:18

Oops we are talking about uname -m. For this, I'm using platform.machine():

conf['Intel?'] = (platform.machine() in ('i386', 'i486', 'i586', 'i686', 'x86_64', 
                                         'AMD64', 'i86pc'))
conf['IA64?']   = (platform.processor() == 'ia64')
conf['PPC?']   = (platform.processor() == 'powerpc')
conf['SPARC?'] = (platform.processor() == 'sparc')

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 3, 2011

comment:19

Replying to @vbraun:

Though I'm not sure if anybody ever verified that ATLAS, with these settings, indeed does not use any more advanced isa extensions.

Regarding that a) the notion of "fat" binaries is misleading (because "fat" originally refers to different code for different [flavours of] processors in the same binary / executable), and b) there's neither a definition nor a consistent practice of what ISA subsets Sage uses if SAGE_FAT_BINARY=yes, I think we can then close this ticket.

If someone complains, we can always change the behaviour of the affected spkg(s); with the exception of AFAIK very rare SIGILL reports for binary distributions Mariah seems to be the only one who actually regularly tests this feature.

We could of course over time collect what different spkgs do, on a wiki page, though.

(For most packages and binary distributions adding -march=... or -mcpu=... to CFLAGS -- or configuring GCC to default to these -- should be sufficient anyway; building binary Sage distributions on less advanced processors is another way.)

@vbraun
Copy link
Member

vbraun commented Aug 3, 2011

comment:20

I agree and will set this ticket to positive review.

Since it is hard/impossible to tell the gcc flags from the compiled binary, I think we should eventually move the SAGE_FAT_BINARY support into the compilerwrapper. It would be easy for the compilerwrapper to set the desired -march / -mtune flags consistently for each binary that we build.

PS: -mcpu is a deprecated synonym for -mtune.

Release manager: this ticket is fixed by #10226 and can be closed.

@vbraun
Copy link
Member

vbraun commented Aug 3, 2011

Reviewer: Leif Leonhardy, Volker Braun

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 3, 2011

comment:21

Replying to @vbraun:

PS: -mcpu is a deprecated synonym for -mtune.

Nope, -mcpu selects the instruction set, while -mtune only affects timing-specific choices (selection or reordering of instructions of that subset); -march=foo is equivalent to -mcpu=foo -mtune=foo. (I don't think that's changed recently, though configure supports --with-arch-NN=... and --with-tune-NN=..., NN in {32, 64}.)


Should we cc Jeroen and delete the ticket from the wishlist wiki page?

@vbraun
Copy link
Member

vbraun commented Aug 3, 2011

comment:22

Well http://gcc.gnu.org/onlinedocs/gcc-4.6.1/gcc/i386-and-x86_002d64-Options.html#i386-and-x86_002d64-Options says:

-mcpu=cpu-type
    A deprecated synonym for -mtune. 

The instruction set is selected with -march.

We should remove the ticket from the wishlist. Jeroen will close this ticket when he gets around to it, no need to contact him.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Aug 3, 2011

comment:23

Replying to @vbraun:

-mcpu=cpu-type
    A deprecated synonym for -mtune. 

Maybe a long-lasting typo (at least since GCC 4.3.3, to be reported upstream ;-) ). Sections on other architectures describe the [IMHO] correct meaning. Or it has been inconsistent for x86 from the beginning, and therefore been deprecated.

@vbraun
Copy link
Member

vbraun commented Aug 3, 2011

comment:24

I don't see any consistency across architectures:

  • x86 has -march and -mtune==-mcpu
  • ARM has -march and -mtune==-mcpu (except perhaps with fewer possible values??)
  • MIPS has -march and -mtune but not -mcpu
  • PowerPC does not have -march, but supports -mtune and -mcpu

There is a ton of dead architectures, but I don't think they have any up-to-date interface.

@jdemeyer
Copy link

jdemeyer commented Aug 3, 2011

Changed author from Mariah Lenox, Willem Jan Palenstijn to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants