[Bug target/64930] New: [5.0 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[Bug target/64930] New: [5.0 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

            Bug ID: 64930
           Summary: [5.0 regression] FAIL: gcc.target/powerpc/atomic-p7.c
                    scan-assembler-times isync 12
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: schwab@linux-m68k.org
            Target: powerpc64-*-*

$ gcc/xgcc -Bgcc/ ../gcc/testsuite/gcc.target/powerpc/atomic-p7.c -mcpu=power7
-O2 -S -m64 -o atomic-p7.s
$ grep -c isync atomic-p7.s
16
$ gcc/xgcc -Bgcc/ ../gcc/testsuite/gcc.target/powerpc/atomic-p8.c -mcpu=power8
-O2 -S -m64 -o atomic-p8.s
$ grep -c isync atomic-p8.s
25

[Bug target/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |5.0
            Summary|[5.0 regression] FAIL:      |[5 regression] FAIL:
                   |gcc.target/powerpc/atomic-p |gcc.target/powerpc/atomic-p
                   |7.c scan-assembler-times    |7.c scan-assembler-times
                   |isync 12                    |isync 12

[Bug target/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
           Priority|P3                          |P1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
smells like wrong-code?

[Bug target/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2015-02-11
                 CC|                            |amacleod at redhat dot com,
                   |                            |amodra at gcc dot gnu.org,
                   |                            |jakub at gcc dot gnu.org,
                   |                            |torvald at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Bisected to r219601 aka PR59448.  I believe that change was intentional, thus
perhaps the testcases should be adjusted for that?

[Bug testsuite/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 34731
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34731&action=edit
gcc5-pr64930.patch

Thus I'm proposing this untested patch.

[Bug testsuite/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[jgreenhalgh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

James Greenhalgh <jgreenhalgh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jgreenhalgh at gcc dot gnu.org

--- Comment #4 from James Greenhalgh <jgreenhalgh at gcc dot gnu.org> ---
This is just the PowerPC equivalent of the FAILs we started seeing on
AArch64/ARM for the same reason (consume promoted to acquire).

Over on the AArch64/ARM side, Mike Stump mentioned that he would prefer the
now-failing search for consume semantics to be marked XFAIL, rather than for us
to update the testcase to reflect the promotion.

https://gcc.gnu.org/ml/gcc-patches/2015-01/msg02517.html

I'm not sure if that changes the approach you want to take for PowerPC,
personally I don't have an opinion either way.

[Bug testsuite/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[torvald at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

--- Comment #5 from torvald at gcc dot gnu.org ---
(In reply to Jakub Jelinek from comment #3)
> Created attachment 34731 [details]
> gcc5-pr64930.patch
> 
> Thus I'm proposing this untested patch.

I think expecting the consume-to-acquire promotion is the right thing to do (I
haven't tested the patch though).  The consume specification in the standard
has issues, and based on the discussions in ISO C++ SG1, it seems unlikely that
it can or will be fixed.  Thus, I believe the promotion will stay with us
forever.

Therefore, using an XFAIL doesn't seem right to me.

[Bug testsuite/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Plus xfailing the isync count test would mean we don't test isync count at all,
even for all the other constructs in the testcase.

[Bug testsuite/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[amodra at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

Alan Modra <amodra at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amodra at gmail dot com

--- Comment #7 from Alan Modra <amodra at gmail dot com> ---
My opinion, FWIW, is that adjusting the expected count of isync is plain wrong.

The pr59448 "fix" effectively means that gcc doesn't support atomic consume, so
I think the testcase should remain failing (maybe add a comment saying why) or
#if 0 out all the consume tests.

[Bug testsuite/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Well, it does support it, but as an alias to acquire.
Keeping the test FAILing is just wrong, and if the promotion of consume to
acquire is going to be permanent or at least for a couple of years, xfail
doesn't make sense either.
We can say in comment that if consume is not promoted to acquire, the count
would be different and how big of course, so if that is ever changed, the test
can be adjusted again.

[Bug testsuite/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[amodra at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

--- Comment #9 from Alan Modra <amodra at gmail dot com> ---
My point was that if you write a testcase that specifically tests for consume
and get acquire code then that is a fail.  The code generated is using a bigger
hammer than necessary.  Imagine for a moment if gcc promoted everything to
seq_cst.  That would also be "supporting" the atomics, but for powerpc we'd be
quite unhappy and would want to know about such a change with a testsuite
failure..

I recognize that the testcase doesn't properly test consume semantics, and that
teaching gcc to track dependencies is not simple.

Huh, I just checked and it appears powerpc emits unnecessary isyncs for
atomic_load consumes prior to the pr59448 change, probably in recognition that
dependency tracking isn't done.  So I withdraw my objection to adjusting the
isync counts.

[Bug testsuite/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Thu Feb 12 13:14:47 2015
New Revision: 220646

URL: https://gcc.gnu.org/viewcvs?rev=220646&root=gcc&view=rev
Log:
    PR testsuite/64930
    * gcc.target/powerpc/atomic-p7.c: Adjust expected count of isync
    instructions for 2015-01-14 get_memmodel changes.
    * gcc.target/powerpc/atomic-p8.c: Likewise.

Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/powerpc/atomic-p7.c
    trunk/gcc/testsuite/gcc.target/powerpc/atomic-p8.c

[Bug testsuite/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.

[Bug testsuite/64930] [5 regression] FAIL: gcc.target/powerpc/atomic-p7.c scan-assembler-times isync 12

[torvald at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64930

--- Comment #12 from torvald at gcc dot gnu.org ---
(In reply to Alan Modra from comment #9)
> My point was that if you write a testcase that specifically tests for
> consume and get acquire code then that is a fail.  The code generated is
> using a bigger hammer than necessary.

The consensus in ISO C++ SG1 (though we had no straw poll on this particular
thing, so this is my conclusion from the discussions and the opinions voiced by
other compiler vendors) is that implementing C/C++ memory_order_consume means,
in practice, promoting to memory_order_acquire.  This is not a GCC-specific
solution or deficiency.  It is rather the realization that the standard's
intent can't be implemented in practice without too much costs elsewhere (e.g.,
because of how tracking dependencies properly would require points-to analysis
or conservatively adding barriers in likely many places in the code).

See this paper for more background:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4321.pdf