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

v2.x: configury: fix asm atomic detection #3039

Merged
merged 2 commits into from
Apr 4, 2017

Conversation

ggouaillardet
Copy link
Contributor

there is no need to look for an assembly file when BUILTIN_GCC is used

Fixes #3032
Refs #3036

Signed-off-by: Gilles Gouaillardet [email protected]

(cherry picked from commit 2f4013c)

there is no need to look for an assembly file when BUILTIN_GCC is used

Fixes open-mpi#3032
Refs open-mpi#3036

Signed-off-by: Gilles Gouaillardet <[email protected]>

(cherry picked from commit open-mpi/ompi@2f4013c)
@ggouaillardet ggouaillardet changed the title configury: fix asm atomic detection v2.x: configury: fix asm atomic detection Feb 27, 2017
@ggouaillardet ggouaillardet added this to the v2.1.0 milestone Feb 27, 2017
@ggouaillardet
Copy link
Contributor Author

@hppritcha @rhc54 this fixes #3032
my analysis is that configure displays a false positive, and GCC builtin atomics are used.
so imho, this is not a blocker, though this would be nice to have when 2.1.0 is released

@ggouaillardet
Copy link
Contributor Author

@hjelmn @rhc54 is opal/asm/asm-data.txt x86_64 is referred as AMD64.
my guess is that comes back from the Opteron debut days.

since this is not specific to AMD processors, should we rename this as X86_64 to avoid any confusion ? (this is a genuine question, and as far as i am concerned, i am fine with AMD64)
if we can reach a consensus, i will be happy to update the PR
fwiw, 32 bits is referred as IA32, so this could also be renamed to X86_32 or something else but vendor neutral.

@rhc54
Copy link
Contributor

rhc54 commented Feb 27, 2017

rename: absolutely! The two archs have diverged over time, and it is definitely misleading and confusing.

@ggouaillardet
Copy link
Contributor Author

done in #3041
will cherry-pick after it passes CI

in this context, AMD64 really means amd64 or em64t, so let's
rename this into X86_64 in order to avoid any confusion

Signed-off-by: Gilles Gouaillardet <[email protected]>

(back-ported from commit open-mpi/ompi@af0b5cf)
@hppritcha
Copy link
Member

Is this fixing a correctness problem that impacts applications?

@hppritcha hppritcha modified the milestones: v2.1.1, v2.1.0 Feb 27, 2017
@rhc54
Copy link
Contributor

rhc54 commented Feb 27, 2017

probably could wait for 2.1.1

@jsquyres
Copy link
Member

Breakdown of the two commits:

  1. The AMD64->x86_64 rename feels cosmetic (misleading, yes, but no one has complained for quite a long time). Feels like it can wait for v2.1.1.
  2. The BUILTIN_GCC fix feels like an optimization -- i.e., prevent some additional configure checks that aren't necessary if we're using builtins. This also feels like it can be deferred to v2.1.1.

Please let me know if these are wrong (i.e., this PR is needed for v2.1.0). Thanks!

@ggouaillardet
Copy link
Contributor Author

@jsquyres the issue was initially reported by @rhc54 on #3032
here is a snippet of the configure output

11:10:41 checking for assembly architecture... AMD64
11:10:41 checking for builtin atomics... BUILTIN_GCC
11:10:41 checking for pre-built assembly file... no (atomic-amd64-linux
11:10:41 amd64-linux-nongas.s not found)
11:10:41 checking whether possible to generate assembly file... failed
11:10:41 configure: WARNING: Could not build atomic operations assembly file.
11:10:41 configure: WARNING: There will be no atomic operations for this build.
11:10:41 checking for atomic assembly filename... none

at first glance, an end user might legitimately think atomic operations could not be built because his/her genuine Intel processor was misidentified as an AMD processor.

the truth is everything is working just fine

  • the processor is detected as a vendor neutral x86_64 (this is fixed by the AMD64-X86_64 rename commit)
  • since BUILTIN_GCC atomics are used, there is no need to use the atomic operations assembly file,
    so there is no need to build it nor display a warning (once again, atomics are built though the warning message says otherwise), and this is fixed by the BUILTIN_GCC commit.

bottom line

  • a bogus and misleading warning message is displayed
  • BUILTIN_GCC atomics are built on x86_64 platforms
  • AMD64 should be read as vendor neutral x86_64

strictly speaking, and imho, this PR is not mandatory for v2.1.0, though it would be nice
to add a few words about it in the README so confused users (if any) can be pointed to.

@jsquyres
Copy link
Member

@ggouaillardet Thanks for the explanation.

@hppritcha Yes, perhaps we should add something to README.

@hppritcha hppritcha merged commit ce9f8ac into open-mpi:v2.x Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants