v3.0.0rc1: XLC-12.1 build errors w/ -q32 and atomics #3816
Comments
|
this is really odd, since out of curiosity, can you please give the inline patch below a try ? diff --git a/opal/include/opal/sys/sync_builtin/atomic.h b/opal/include/opal/sys/sync_builtin/atomic.h
index 0f18039..b52e82f 100644
--- a/opal/include/opal/sys/sync_builtin/atomic.h
+++ b/opal/include/opal/sys/sync_builtin/atomic.h
@@ -13,6 +13,8 @@
* Copyright (c) 2011 Sandia National Laboratories. All rights reserved.
* Copyright (c) 2014-2016 Los Alamos National Security, LLC. All rights
* reserved.
+ * Copyright (c) 2017 Research Organization for Information Science
+ * and Technology (RIST). All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
@@ -30,17 +32,17 @@
*********************************************************************/
#define OPAL_HAVE_ATOMIC_MEM_BARRIER 1
-static inline void opal_atomic_mb(void)
+static inline void opal_atomic_mb()
{
__sync_synchronize();
}
-static inline void opal_atomic_rmb(void)
+static inline void opal_atomic_rmb()
{
__sync_synchronize();
}
-static inline void opal_atomic_wmb(void)
+static inline void opal_atomic_wmb()
{
__sync_synchronize();
} |
|
I can verify that the following 4-line C program compiles fine with the same arguments to xlc other than the -o: So, I don't think the configure check was wrong. Even without the patch above from @ggouaillardet to remove With the patch, the one observed failure on ompi/mpi/c/profile/paddress.c is fixed. Makes no sense to me. |
just declare inline subroutines with static inline ... () in order to make some compilers (xlc 12.1) happy Thanks Paul Hargrove for the report Refs. open-mpi#3816 Signed-off-by: Gilles Gouaillardet <[email protected]>
|
@PHHargrove Just curious: is I ask because if it first shows up in |
|
@jsquyres This is not problem with LP64 (the preferred ABI) and I really think this looks to be a compiler bug. While 12.1 is the latest XLC on this system (the front-end to a BG/Q), it is not the latest release for big-endian PPC (which is 13.x). |
|
I agree with @PHHargrove; I'll put together a README note this afternoon. |
|
@bwbarrett |
|
I asked our compiler person and he said: By looking at the code change, it seems that the failure is caused by something else. In the C standard, there is no difference between foo() {...} and foo(void) {...} (i.e. function definitions), both mean that the function has no parameter. However, there is a difference between foo() and foo(void). Two relevant references in the C11 standard:
|
|
@PHHargrove , yeah, I'd suspect it's the builtin atomics, since those are new to the 3.0.x series. Will wait for your feedback. |
|
I like the README with possible workaround of disabling atomics in README. |
|
@gpaulsen |
|
@bwbarrett |
|
So how should we resolve this? I'd think that compiler generated atomics for -q32 case would be acceptable. Do we just need to add some configure logic to make that happen? |
|
It might be worthwhile to put in a specific configure test for this broken compiler scenario. |
|
@jsquyres wrote:
Based on the odd circumstances (only the profile build, and not until the 5th file), I doubt anything can be done at configure time other than explicitly identifying the compiler by version and the ABI from something like sizeof(void*). |
|
@bwbarrett Adding the target-v3.x tag because I think we have a simple fix that can be included in v3.0.0. |
|
Opps, wrong one. |
|
@bwbarrett This one will be fixed in v3.0.1 when we disable the builtin atomics by default again. Are we just waiting on documenting this issue for v3.0.0? |
|
why are we waiting for 3.0.1 to disable builtin atomics? If it's broken, we should fix it before we release... |
|
@bwbarrett wrote
Because there are trolls living under that particular bridge. |
|
@bwbarrett We discussed just this at the developer meeting (or on the phone-- can't remember). It is deemed too late in the release to change it. |
|
#4001 would indeed need to be fixed though..... |
|
So this is a 3.0.0 or not for 3.0.x decision; it's absolutely the wrong thing to plan on swapping something like atomics around mid-release series. I'm not even sure we should enable built-in for 3.0 and then disable it for 3.1; that feels like the wrong thing for our users. |
|
@bwbarrett Exactly. I would be more comfortable not using the builtins. They are slower right now and we haven't determined why. FWIW, it is a single line change to configury to go back to the v2.x default. |
|
I think it really is time to kill the pre-built assembly. I don't know of any work done on supporting that code in the last couple of years. Killing that would help fix #4001. |
|
now that we require perl (sigh), the pre-built stuff can die. It was a workaround for us not wanting to require perl. |
|
@hjelmn, do you want to put together the build-ins not by default patch or should I? |
|
@bwbarrett I will put it together. |
|
This has been resolved by PR #4103 |
With xlc-12.1 on a ppc64 host, I am trying to build for ILP32, and configure as follows:
This build was previously failing due to #3811, but a patch from @ggouaillardet gets me past that and on to the next problem:
All thee errors are on lines with just
__sync_synchronize().I have attached the config.log
The text was updated successfully, but these errors were encountered: