diff mbox

Re: [PATCH][AArch64] Testcase fix for __ATOMIC_CONSUME

Message ID 54D8CE31.6090503@arm.com
State New
Headers show

Commit Message

Alex Velenko Feb. 9, 2015, 3:11 p.m. UTC 
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.

+/* { 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--

Comments

Mike Stump Feb. 9, 2015, 5:10 p.m. UTC | #1 
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. */
Torvald Riegel Feb. 11, 2015, 8:16 p.m. UTC | #2 
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.
Mike Stump Feb. 12, 2015, 6:38 p.m. UTC | #3 
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.
diff mbox

Patch

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.  */