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 spkg does not take the SAGE_ATLAS_ARCH variable into account when SAGE_FAT_BINARY is set to 'yes' #13706

Closed
sagetrac-tmonteil mannequin opened this issue Nov 13, 2012 · 25 comments

Comments

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Nov 13, 2012

The spkg-install code of atlas spkg is structured as follow (from line 192):

if SAGE_FAT_BINARY == 'yes':
    arch = 'HAMMER'
elif SAGE_ATLAS_ARCH is defined:
    arch = 'SAGE_ATLAS_ARCH'

It has the effect not to take the SAGE_ATLAS_ARCH variable into account when SAGE_FAT_BINARY is set to 'yes'. I consider it as a bug, since on may want to build a fat binary for an architecture that is not HAMMER (for example, one may want to build a fat binary able to run on Pentium 3).

Component: build

Keywords: atlas, days43, denigration, community, management, sdl

Reviewer: Volker Braun

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

@sagetrac-tmonteil sagetrac-tmonteil mannequin added this to the sage-5.10 milestone Nov 13, 2012
@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Nov 13, 2012

Attachment: trac_13706-atlas_arch-tm.patch.gz

proposal minimalistic patch

@sagetrac-tmonteil

This comment has been minimized.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Nov 13, 2012

comment:2

May it be cleaner to switch the two conditions instead of replacing the 'elif' by a 'if' ? Also, why was the arch 'HAMMER' chosen by default ?

@vbraun
Copy link
Member

vbraun commented Nov 15, 2012

comment:3

#10508 uses the new x86x87 target for building ATLAS on i386 with SAGE_FAT_BINARY, so I'm pretty sure that this has been fixed for a while now.

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Nov 15, 2012

comment:4

As of today, this seems not to be fixed in the spkg proposed in #10508 : SAGE_FAT_BINARY default still overwrites SAGE_ATLAS_ARCH when it is set.

SAGE_FAT_BINARY is not only used for atlas, but also for (according to grep in the sage-5.4 sources) : ecm-6.3.p8 libm4ri-20120613 mpir-2.4.0.p6 polybori-0.8.2 r-2.14.0.p6 hence one may want to take advantage of it and be more precise by setting the SAGE_ATLAS_ARCH variable.

By the way, it is interesting to see that, when SAGE_FAT_BINARY=='yes' in sage-5.4, libm4ri-20120613 explicitely disables SSE2 set of instructions and atlas-3.8.4.p1 explicitely enables it.

Also, in the #10508 package, configure_base() method adds 3DNow set of instructions to some Intel architecture, which seems not to know it: https://en.wikipedia.org/wiki/3DNow!

@vbraun
Copy link
Member

vbraun commented Nov 15, 2012

comment:5

SAGE_FAT_BINARY default still overwrites SAGE_ATLAS_ARCH when it is set.

The combination is nonsensical: You either want a binary that runs on all reasonably old hardware for distribution, or you want to specify the architecture in detail. Which one is it? I don't mind adding support for nonsensical combinations if there is demand. But the only bug here is that it requires SSE2 on i386, which is probably too much. Note that the old ATLAS (which we currently ship, and probably will for a while) doesn't have "generic" archdefs to start with so it always was a crapshot.

Also, in the #10508 package, configure_base() method adds 3DNow set of
instructions to some Intel architecture, which seems not to know it:
https://en.wikipedia.org/wiki/3DNow!

So? Sage will never run with only the original 8086 instruction set. For many of the processors on your web page you'll have to recompile a linux distro from source, never mind Sage.

@vbraun
Copy link
Member

vbraun commented Dec 18, 2012

comment:6

Close as invalid since everybody seems to agree that the combination in nonsensical and the SSE2 issue is addressed at #10508.

@vbraun
Copy link
Member

vbraun commented Dec 18, 2012

Author: Volker Braun

@jdemeyer
Copy link
Contributor

Changed author from Volker Braun to none

@jdemeyer
Copy link
Contributor

Reviewer: Volker Braun

@jdemeyer jdemeyer removed this from the sage-5.10 milestone May 19, 2013
@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented May 19, 2013

comment:8

I disagree with this positive review.

  • "everybody seems to agree that the combination in nonsensical" -> Citation needed !

  • "You either want a binary that runs on all reasonably old hardware for distribution, or you want to specify the architecture in detail" -> i want both, that is why local and global variables are made for !

  • The fact that the cross-packages SAGE_FAT_BINARY variable takes precedence over the atlas-specific variable SAGE_ATLAS_ARCH is not addressed in Update ATLAS to stable version 3.10.1 #10508.

  • Currently, if i want to tune atlas for a specific old architecture, and at the same time tell other packages not to use too recent sets of instructions (which is coherent), i simply can not. I still consider this as a bug. It makes all the predefined atlas architectures settings useless: if i want to specify one of them, i can not use SAGE_FAT_BINARY for the other packages, which impossible for an old architecture.

@sagetrac-tmonteil sagetrac-tmonteil mannequin added the pending label May 19, 2013
@vbraun
Copy link
Member

vbraun commented May 19, 2013

comment:9

You can recompile atlas after installing sage using the atlas-config script. This allows your obscure use case without burdening the process for making binary distributions. As a plus you'll actually have a chance of getting the correct tuning values since the script can disable CPU sleep states.

#10508 addresses the issue presented here in that we don't require SSE for generic i386 builds any more.

@vbraun vbraun removed the pending label May 19, 2013
@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented May 19, 2013

Changed keywords from atlas, days43 to atlas, days43 denigration community management

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented May 19, 2013

comment:10

Understood, i won't bother anymore about this.

@jdemeyer
Copy link
Contributor

comment:11

Replying to @sagetrac-tmonteil:

  • "You either want a binary that runs on all reasonably old hardware for distribution, or you want to specify the architecture in detail" -> i want both, that is why local and global variables are made for !

I don't understand what "both" could possibly mean. How can it run on all architectures while still being optimized for one architecture? If it needs to run on all architectures, it cannot use Core2-specific instructions for example, so how could be optimized for Core2?

Currently, if i want to tune atlas for a specific old architecture, and at the same time tell other packages not to use too recent sets of instructions (which is coherent), i simply can not.

  1. If the "specific old architecture" is the one you're building on, it's easy: don't set any environment variable and build as usual.

  2. If the "specific old architecture" is not the one you're building on but you still don't want a generic binary: first of all, why would you want that? But this can probably be achieved by setting specific CFLAGS, although this isn't really supported.

@jdemeyer
Copy link
Contributor

comment:12

No answer, closing this.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jun 17, 2013

comment:13

Replying to @jdemeyer:

Replying to @sagetrac-tmonteil:

  • "You either want a binary that runs on all reasonably old hardware for distribution, or you want to specify the architecture in detail" -> i want both, that is why local and global variables are made for !

I don't understand what "both" could possibly mean.

Well, he apparently meant he wants to specify what "reasonably old" means to him (or, more precisely, what's needed / more appropriate in his use case).

To me doesn't sound too exotic or "obscure"...

@vbraun
Copy link
Member

vbraun commented Jun 17, 2013

comment:14

You can (and always could) specify precisely what you want for ATLAS using SAGE_ATLAS_ARCH.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jun 17, 2013

comment:15

Replying to @vbraun:

You can (and always could) specify precisely what you want for ATLAS using SAGE_ATLAS_ARCH.

... unless you also specified SAGE_FAT_BINARY=yes, which is what this ticket was all about. ;-)

@vbraun
Copy link
Member

vbraun commented Jun 17, 2013

comment:16

That is because SAGE_FAT_BINARY really means: give me what Sage considers to be sufficiently generic to run on a variety of old hardware.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jun 17, 2013

comment:17

Replying to @vbraun:

That is because SAGE_FAT_BINARY really means: give me what Sage considers to be sufficiently generic to run on a variety of old hardware.

.. which is a misinterpretation of "fat".

@vbraun
Copy link
Member

vbraun commented Jun 17, 2013

comment:18

If you are complaining about the misnomer then you are quite late to the party ;-)

@nexttime
Copy link
Mannequin

nexttime mannequin commented Jun 17, 2013

comment:19

Replying to @vbraun:

If you are complaining about the misnomer then you are quite late to the party ;-)

You know, je später die Gäste...

(Although that's not the first time I'm arguing that way.)

The MPIR spkg supports exactly what's meant by "fat binary", although on x86 / x86_64 only IIRC.

@jdemeyer
Copy link
Contributor

comment:20

For leif, we should support the variable

SAGE_FAT_BINARY_OR_GENERIC_BINARY_DEPENDING_ON_THE_PACKAGE_AND_PROCESSOR=yes

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Aug 27, 2019

Changed keywords from atlas, days43 denigration community management to atlas, days43, denigration, community, management, sdl

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

2 participants