* [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME
@ 2015-01-27 17:08 Alex Velenko
2015-01-28 18:13 ` Mike Stump
0 siblings, 1 reply; 10+ messages in thread
From: Alex Velenko @ 2015-01-27 17:08 UTC (permalink / raw)
To: gcc-patches; +Cc: marcus.shawcroft
[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]
Hi,
This patch fixes aarch64/atomic-op-consume.c test to expect safe "LDAXR"
instruction to be generated when __ATOMIC_CONSUME semantics is requested.
This patch was tested by running the modified test on aarch64-none-elf
compiler.
Is this patch ok?
Alex
2015-01-27 Alex Velenko <Alex.Velenko@arm.com>
gcc/testsuite/
* gcc.target/aarch64/atomic-op-consume.c (scan-assember-times): Adjust
scan-assembler-times pattern.
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 } } */
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME
2015-01-27 17:08 [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME Alex Velenko
@ 2015-01-28 18:13 ` Mike Stump
2015-01-28 18:15 ` Marcus Shawcroft
0 siblings, 1 reply; 10+ messages in thread
From: Mike Stump @ 2015-01-28 18:13 UTC (permalink / raw)
To: Alex Velenko; +Cc: gcc-patches, marcus.shawcroft
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME
2015-01-28 18:13 ` Mike Stump
@ 2015-01-28 18:15 ` Marcus Shawcroft
2015-01-28 18:29 ` James Greenhalgh
2015-01-28 19:55 ` Mike Stump
0 siblings, 2 replies; 10+ messages in thread
From: Marcus Shawcroft @ 2015-01-28 18:15 UTC (permalink / raw)
To: Mike Stump; +Cc: Alex Velenko, gcc-patches, Marcus Shawcroft
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME
2015-01-28 18:15 ` Marcus Shawcroft
@ 2015-01-28 18:29 ` James Greenhalgh
2015-01-28 19:55 ` Mike Stump
1 sibling, 0 replies; 10+ messages in thread
From: James Greenhalgh @ 2015-01-28 18:29 UTC (permalink / raw)
To: Marcus Shawcroft; +Cc: Mike Stump, Alex Velenko, gcc-patches, Marcus Shawcroft
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME
2015-01-28 18:15 ` Marcus Shawcroft
2015-01-28 18:29 ` James Greenhalgh
@ 2015-01-28 19:55 ` Mike Stump
2015-02-09 15:11 ` Alex Velenko
1 sibling, 1 reply; 10+ messages in thread
From: Mike Stump @ 2015-01-28 19:55 UTC (permalink / raw)
To: Marcus Shawcroft; +Cc: Alex Velenko, gcc-patches, Marcus Shawcroft
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME
2015-01-28 19:55 ` Mike Stump
@ 2015-02-09 15:11 ` Alex Velenko
2015-02-09 17:10 ` Mike Stump
0 siblings, 1 reply; 10+ messages in thread
From: Alex Velenko @ 2015-02-09 15:11 UTC (permalink / raw)
To: Mike Stump, Marcus Shawcroft; +Cc: gcc-patches, Marcus Shawcroft
On 28/01/15 18:50, Mike Stump wrote:
> 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.
>
Hi, Mike!
Sorry for the delay.
The following patch makes atomic-op-consume.c XFAIL for both
gcc.target/arm and gcc.target/aarch64 tests. XFAIL was chosen as
prefered approach for handling __ATOMIC_CONSUME to __ATOMIC_ACQUIRE
promotion workaround.
This patch was tested by running the modified tests on aarch64-none-elf
and arm-none-eabi compilers.
Is this patch ok?
Alex
2015-02-09 Alex Velenko <Alex.Velenko@arm.com>
gcc/testsuite/
* gcc.target/aarch64/atomic-op-consume.c (scan-assember-times):
Directive adjusted to XFAIL.
* gcc.target/arm/atomic-op-consume.c (scan-assember-times): Directive
adjusted to XFAIL.
diff --git a/gcc/testsuite/gcc.target/aarch64/atomic-op-consume.c
b/gcc/testsuite/gcc.target/aarch64/atomic-op-consume.c
index 38d6c2c..8150af6 100644
--- a/gcc/testsuite/gcc.target/aarch64/atomic-op-consume.c
+++ b/gcc/testsuite/gcc.target/aarch64/atomic-op-consume.c
@@ -3,5 +3,9 @@
#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". Therefore, "LDXR" test is
+ expected to fail. */
+/* { dg-final { scan-assembler-times "ldxr\tw\[0-9\]+,
\\\[x\[0-9\]+\\\]" 6 { xfail *-*-* } } } */
/* { dg-final { scan-assembler-times "stxr\tw\[0-9\]+, w\[0-9\]+,
\\\[x\[0-9\]+\\\]" 6 } } */
diff --git a/gcc/testsuite/gcc.target/arm/atomic-op-consume.c
b/gcc/testsuite/gcc.target/arm/atomic-op-consume.c
index cc6c028..060655c 100644
--- a/gcc/testsuite/gcc.target/arm/atomic-op-consume.c
+++ b/gcc/testsuite/gcc.target/arm/atomic-op-consume.c
@@ -7,7 +7,8 @@
/* To workaround Bugzilla 59448 issue, a request for __ATOMIC_CONSUME
is always
promoted to __ATOMIC_ACQUIRE, implemented as MEMMODEL_ACQUIRE.
This causes
- "LDAEX" to be generated instead of "LDREX". */
-/* { dg-final { scan-assembler-times "ldaex\tr\[0-9\]+,
\\\[r\[0-9\]+\\\]" 6 } } */
+ "LDAEX" to be generated instead of "LDREX". Therefore, "LDREX" test is
+ expected to fail. */
+/* { dg-final { scan-assembler-times "ldrex\tr\[0-9\]+,
\\\[r\[0-9\]+\\\]" 6 { xfail *-*-* } } } */
/* { dg-final { scan-assembler-times "strex\t...?, r\[0-9\]+,
\\\[r\[0-9\]+\\\]" 6 } } */
/* { dg-final { scan-assembler-not "dmb" } } */
--------------1.8.1.2--
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME
2015-02-09 15:11 ` Alex Velenko
@ 2015-02-09 17:10 ` Mike Stump
2015-02-11 20:17 ` Torvald Riegel
0 siblings, 1 reply; 10+ messages in thread
From: Mike Stump @ 2015-02-09 17:10 UTC (permalink / raw)
To: Alex Velenko; +Cc: Marcus Shawcroft, gcc-patches, Marcus Shawcroft
On Feb 9, 2015, at 7:11 AM, Alex Velenko <Alex.Velenko@arm.com> wrote:
> The following patch makes atomic-op-consume.c XFAIL
>
> Is this patch ok?
Ok.
I’d shorten the comment above the xfail to be exceedingly short:
/* PR59448 consume not implemented yet */
The reason is the brain can process this about 8x faster. Also, one can cut and paste the PR part into a web browser directly, or, if you have an electric bugzilla mode for emacs, it will pop right up. */
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME
2015-02-09 17:10 ` Mike Stump
@ 2015-02-11 20:17 ` Torvald Riegel
2015-02-12 18:38 ` Mike Stump
0 siblings, 1 reply; 10+ messages in thread
From: Torvald Riegel @ 2015-02-11 20:17 UTC (permalink / raw)
To: Mike Stump; +Cc: Alex Velenko, Marcus Shawcroft, gcc-patches, Marcus Shawcroft
On Mon, 2015-02-09 at 09:10 -0800, Mike Stump wrote:
> On Feb 9, 2015, at 7:11 AM, Alex Velenko <Alex.Velenko@arm.com> wrote:
> > The following patch makes atomic-op-consume.c XFAIL
> >
> > Is this patch ok?
>
> Ok.
>
> Iâd shorten the comment above the xfail to be exceedingly short:
>
> /* PR59448 consume not implemented yet */
>
> The reason is the brain can process this about 8x faster. Also, one can cut and paste the PR part into a web browser directly, or, if you have an electric bugzilla mode for emacs, it will pop right up. */
Given the discussions we had in ISO C++ SG1, it seems the only way to
fix memory_order_consume is to deprecate it (or let it rot forever), and
add something else to the standard. See
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4321.pdf
IOW, I believe the promotion is here to stay. I'm not aware of any
other implementation doing something else.
Thus, XFAIL doesn't seem right to me.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME
2015-02-11 20:17 ` Torvald Riegel
@ 2015-02-12 18:38 ` Mike Stump
2015-02-18 15:43 ` Alex Velenko
0 siblings, 1 reply; 10+ messages in thread
From: Mike Stump @ 2015-02-12 18:38 UTC (permalink / raw)
To: Torvald Riegel
Cc: Alex Velenko, Marcus Shawcroft, gcc-patches, Marcus Shawcroft
On Feb 11, 2015, at 12:16 PM, Torvald Riegel <triegel@redhat.com> wrote:
> On Mon, 2015-02-09 at 09:10 -0800, Mike Stump wrote:
>> On Feb 9, 2015, at 7:11 AM, Alex Velenko <Alex.Velenko@arm.com> wrote:
>>> The following patch makes atomic-op-consume.c XFAIL
>>>
>>> Is this patch ok?
>>
>> Ok.
>>
>> I’d shorten the comment above the xfail to be exceedingly short:
>>
>> /* PR59448 consume not implemented yet */
>>
>> The reason is the brain can process this about 8x faster. Also, one can cut and paste the PR part into a web browser directly, or, if you have an electric bugzilla mode for emacs, it will pop right up. */
>
> Given the discussions we had in ISO C++ SG1, it seems the only way to
> fix memory_order_consume is to deprecate it (or let it rot forever), and
> add something else to the standard. See
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4321.pdf
Nice paper, thanks.
> IOW, I believe the promotion is here to stay. I'm not aware of any
> other implementation doing something else.
>
> Thus, XFAIL doesn't seem right to me.
Since Jakub in PR64930 updated to the now expected output instead of xfail, and given the paper above, easy to agree with this. The changes to remove the xfail and expect the now expected codegen are pre-approved.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME
2015-02-12 18:38 ` Mike Stump
@ 2015-02-18 15:43 ` Alex Velenko
0 siblings, 0 replies; 10+ messages in thread
From: Alex Velenko @ 2015-02-18 15:43 UTC (permalink / raw)
To: Mike Stump, Torvald Riegel
Cc: Marcus Shawcroft, gcc-patches, Marcus Shawcroft
On 12/02/15 18:38, Mike Stump wrote:
> On Feb 11, 2015, at 12:16 PM, Torvald Riegel <triegel@redhat.com> wrote:
>> On Mon, 2015-02-09 at 09:10 -0800, Mike Stump wrote:
>>> On Feb 9, 2015, at 7:11 AM, Alex Velenko <Alex.Velenko@arm.com> wrote:
>>>> The following patch makes atomic-op-consume.c XFAIL
>>>>
>>>> Is this patch ok?
>>>
>>> Ok.
>>>
>>> I’d shorten the comment above the xfail to be exceedingly short:
>>>
>>> /* PR59448 consume not implemented yet */
>>>
>>> The reason is the brain can process this about 8x faster. Also, one can cut and paste the PR part into a web browser directly, or, if you have an electric bugzilla mode for emacs, it will pop right up. */
>>
>> Given the discussions we had in ISO C++ SG1, it seems the only way to
>> fix memory_order_consume is to deprecate it (or let it rot forever), and
>> add something else to the standard. See
>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4321.pdf
>
> Nice paper, thanks.
>
>> IOW, I believe the promotion is here to stay. I'm not aware of any
>> other implementation doing something else.
>>
>> Thus, XFAIL doesn't seem right to me.
>
> Since Jakub in PR64930 updated to the now expected output instead of xfail, and given the paper above, easy to agree with this. The changes to remove the xfail and expect the now expected codegen are pre-approved.
>
>
Hi Mike,
As pre-approved trivial change, on Monday I commited the following patch:
gcc/testsuite/
2015-02-16 Alex Velenko <Alex.Velenko@arm.com>
* gcc.target/aarch64/atomic-op-consume.c (scan-assember-times):
Directive adjusted to scan for ldaxr.
* gcc.target/arm/atomic-op-consume.c (scan-assember-times): Directive
adjusted to scan for ldaex.
diff --git a/gcc/testsuite/gcc.target/aarch64/atomic-op-consume.c
b/gcc/testsuite/gcc.target/aarch64/atomic-op-consume.c
index 0e6dbbe..26ebbdf 100644
--- a/gcc/testsuite/gcc.target/aarch64/atomic-op-consume.c
+++ b/gcc/testsuite/gcc.target/aarch64/atomic-op-consume.c
@@ -3,6 +3,6 @@
#include "atomic-op-consume.x"
-/* PR59448 consume not implemented yet. */
-/* { dg-final { scan-assembler-times "ldxr\tw\[0-9\]+,
\\\[x\[0-9\]+\\\]" 6 { xfail *-*-* } } } */
+/* Scan for ldaxr is a PR59448 consume workaround. */
+/* { 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 } } */
diff --git a/gcc/testsuite/gcc.target/arm/atomic-op-consume.c
b/gcc/testsuite/gcc.target/arm/atomic-op-consume.c
index fafe4d6..6c5f989 100644
--- a/gcc/testsuite/gcc.target/arm/atomic-op-consume.c
+++ b/gcc/testsuite/gcc.target/arm/atomic-op-consume.c
@@ -5,7 +5,7 @@
#include "../aarch64/atomic-op-consume.x"
-/* PR59448 consume not implemented yet. */
-/* { dg-final { scan-assembler-times "ldrex\tr\[0-9\]+,
\\\[r\[0-9\]+\\\]" 6 { xfail *-*-* } } } */
+/* Scan for ldaex is a PR59448 consume workaround. */
+/* { dg-final { scan-assembler-times "ldaex\tr\[0-9\]+,
\\\[r\[0-9\]+\\\]" 6 } } */
/* { dg-final { scan-assembler-times "strex\t...?, r\[0-9\]+,
\\\[r\[0-9\]+\\\]" 6 } } */
/* { dg-final { scan-assembler-not "dmb" } } */
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-02-18 15:43 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-27 17:08 [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME Alex Velenko
2015-01-28 18:13 ` Mike Stump
2015-01-28 18:15 ` Marcus Shawcroft
2015-01-28 18:29 ` James Greenhalgh
2015-01-28 19:55 ` Mike Stump
2015-02-09 15:11 ` Alex Velenko
2015-02-09 17:10 ` Mike Stump
2015-02-11 20:17 ` Torvald Riegel
2015-02-12 18:38 ` Mike Stump
2015-02-18 15:43 ` Alex Velenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).