opal_atomic_rmb: GCC has fixed its acquire fence barrier in its 8 release #10118
Conversation
|
Forgot to add: I came across this issue in the test suite of another project with GCC 7.3.0. With GCC 8.1.0 the issue seems to be fixed and the test suite succeeds there. We should only exclude GCC prior to 8.1.0 and not all x86-64 compilers. |
opal/include/opal/sys/atomic_stdc.h
Outdated
| */ | ||
| # if defined(PLATFORM_ARCH_X86_64) && \ | ||
| PLATFORM_COMPILER_GNU && __GNUC__ < 8 | ||
| # define OPAL_ATOMIC_RMB_GCC_FENCE_ACQUIRE_BROKEN 1 |
There was a problem hiding this comment.
why do we need the extra define? I'd rather not create another (in this case atomic_stdc-only) define we need to maintain going forward.
There was a problem hiding this comment.
I thought it'd be cleaner (I had a somewhat more complex condition earlier). I agree, put it back into the code directly.
opal/include/opal/sys/atomic_stdc.h
Outdated
| * to be fixed from 8.1.0 onwards | ||
| */ | ||
| # if defined(PLATFORM_ARCH_X86_64) && \ | ||
| PLATFORM_COMPILER_GNU && __GNUC__ < 8 |
There was a problem hiding this comment.
Is the issue fixed in GCC 8.0 (the code) or GCC 8.1 (the comment)? We should be careful there. Do we need to check __GNUC_MINOR__ as well?
There was a problem hiding this comment.
There was no 8.0 release. The first release in the 8 series was 8.1: https://gcc.gnu.org/releases.html
There was a problem hiding this comment.
That would be a good thing to put in a comment.
There was a problem hiding this comment.
Done, took out the reference to 8.1.0 and mention 8 release series instead.
devreal
force-pushed
the
atomic_acq_fence_fix
branch
3 times, most recently
from
ca1f77f to
b2f9f7e
Compare
opal/include/opal/sys/atomic_stdc.h
Outdated
| @@ -48,12 +48,17 @@ static inline void opal_atomic_wmb(void) | |||
| atomic_thread_fence(memory_order_release); | |||
| } | |||
|
|
|||
| # endif | |||
There was a problem hiding this comment.
I think you left this extra #endif from the previous version of the patch?
devreal
force-pushed
the
atomic_acq_fence_fix
branch
from
b2f9f7e to
e5d2c2a
Compare
|
For the record, this issue was brought up and fixed in a 2017 GCC bug report. The issue had already been reported back in 2012 but little details were provided, so it went to /dev/null. Lesson learned: proper issue reporting could have saved 5 years of broken acquire fence and workarounds in 3rd party codes that will stay around for at least a decade... |
…ease Only force a full memory barrier for GCC releases prior to 8.1.0. Signed-off-by: Joseph Schuchart <schuchart@icl.utk.edu>
devreal
force-pushed
the
atomic_acq_fence_fix
branch
from
e5d2c2a to
935ee76
Compare
|
I put the link to the GCC bug report into the comment. I think this is good to go. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Did anyone test any non-gcc compilers on x86? This will change the code-path for icc and other compilers on x86 as well. The premise that this is only a gcc bug in versions <= 7, but the previous code would have hidden any bugs in other compilers on x86. |
Only force a full memory barrier for GCC releases prior to 8.1.0.
Signed-off-by: Joseph Schuchart schuchart@icl.utk.edu