| Message ID | 1422375847-18680-1-git-send-email-alex.velenko@arm.com |
|---|---|
| State | New |
| Headers | show |
On Jan 27, 2015, at 8:24 AM, Alex Velenko <Alex.Velenko@arm.com> wrote: > This patch fixes aarch64/atomic-op-consume.c test to expect safe "LDAXR" > instruction to be generated when __ATOMIC_CONSUME semantics is requested. Did you see: /* Workaround for Bugzilla 59448. GCC doesn't track consume properly, so be conservative and promote consume to acquire. */ if (val == MEMMODEL_CONSUME) val = MEMMODEL_ACQUIRE; in builtins.c? Feels like if gcc isn’t going to support it for you, then testing for it would be, hard?
On 28 January 2015 at 17:41, Mike Stump <mikestump@comcast.net> wrote: > On Jan 27, 2015, at 8:24 AM, Alex Velenko <Alex.Velenko@arm.com> wrote: >> This patch fixes aarch64/atomic-op-consume.c test to expect safe "LDAXR" >> instruction to be generated when __ATOMIC_CONSUME semantics is requested. > > Did you see: > > /* Workaround for Bugzilla 59448. GCC doesn't track consume properly, so > be conservative and promote consume to acquire. */ > if (val == MEMMODEL_CONSUME) > val = MEMMODEL_ACQUIRE; > > in builtins.c? Feels like if gcc isn’t going to support it for you, then testing for it would be, hard? The original test was written pre 59448 and expects GCC to implement consume behaviour. The workaround for 59448 changes GCC behaviour but did not update this test case. Going forward we can either remove the test case completely, xfail the test case pending a proper solution to 59448 ? or change the test case to expect the current intended behaviour of gcc. This patch implements that latter, which seems reasonable to me. Mike do you prefer one of the other two approaches ? Cheers /Marcus
On Wed, Jan 28, 2015 at 05:51:27PM +0000, Marcus Shawcroft wrote: > On 28 January 2015 at 17:41, Mike Stump <mikestump@comcast.net> wrote: > > On Jan 27, 2015, at 8:24 AM, Alex Velenko <Alex.Velenko@arm.com> wrote: > >> This patch fixes aarch64/atomic-op-consume.c test to expect safe "LDAXR" > >> instruction to be generated when __ATOMIC_CONSUME semantics is requested. > > > > Did you see: > > > > /* Workaround for Bugzilla 59448. GCC doesn't track consume properly, so > > be conservative and promote consume to acquire. */ > > if (val == MEMMODEL_CONSUME) > > val = MEMMODEL_ACQUIRE; > > > > in builtins.c? Feels like if gcc isn’t going to support it for you, then testing for it would be, hard? > > The original test was written pre 59448 and expects GCC to implement > consume behaviour. The workaround for 59448 changes GCC behaviour but > did not update this test case. Going forward we can either remove the > test case completely, xfail the test case pending a proper solution to > 59448 ? or change the test case to expect the current intended > behaviour of gcc. This patch implements that latter, which seems > reasonable to me. Mike do you prefer one of the other two approaches ? I'll vote for keeping the test case, please. It still fulfills a useful purpose. GCC must now promote __ATOMIC_CONSUME to __ATOMIC_ACQUIRE, and the test case now reflects that - expecting the LDAXR instruction (load acquire exclusive) over the more relaxed LDXR (load exclusive) that we emitted prior to PR59448. If we lose that promotion, or if we start emitting different instructions for __ATOMIC_ACQUIRE, we have a regression, and this test should spot that. I also wouldn't want to see the test XFAIL; we know what the correct expected behaviour is and should update the test to reflect that - as in Alex' patch. Thanks, James
On Jan 28, 2015, at 9:51 AM, Marcus Shawcroft <marcus.shawcroft@gmail.com> wrote: > Going forward we can [ … ] xfail the test case pending a proper solution to > 59448 ? > Mike do you prefer one of the other two approaches ? I’d xfail the test case and mark with the fix consume PR. If we don’t have an unambiguous, fix consume PR, I’d file that. It should be listed as failing on aarch, and the fix for that PR should then make the aarch test case pass. This way no one can run off with the PR and do anything else with it.
diff --git a/gcc/testsuite/gcc.target/aarch64/atomic-op-consume.c b/gcc/testsuite/gcc.target/aarch64/atomic-op-consume.c index 38d6c2c..7ece5b1 100644 --- a/gcc/testsuite/gcc.target/aarch64/atomic-op-consume.c +++ b/gcc/testsuite/gcc.target/aarch64/atomic-op-consume.c @@ -3,5 +3,8 @@ #include "atomic-op-consume.x" -/* { dg-final { scan-assembler-times "ldxr\tw\[0-9\]+, \\\[x\[0-9\]+\\\]" 6 } } */ +/* To workaround Bugzilla 59448 issue, a request for __ATOMIC_CONSUME is always + promoted to __ATOMIC_ACQUIRE, implemented as MEMMODEL_ACQUIRE. This causes + "LDAXR" to be generated instead of "LDXR". */ +/* { dg-final { scan-assembler-times "ldaxr\tw\[0-9\]+, \\\[x\[0-9\]+\\\]" 6 } } */ /* { dg-final { scan-assembler-times "stxr\tw\[0-9\]+, w\[0-9\]+, \\\[x\[0-9\]+\\\]" 6 } } */