[Bug target/55492] New: __atomic_load doesn't match ACQUIRE memory model

[Bug target/55492] New: __atomic_load doesn't match ACQUIRE memory model

[yvan.roux at linaro dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55492

             Bug #: 55492
           Summary: __atomic_load doesn't match ACQUIRE memory model
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: yvan.roux@linaro.org


Created attachment 28796
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=28796
Generic expansion patch proposal

Compiling this code for ARMv7

int v;

int foo()
{
  return __atomic_load_n (&v, __ATOMIC_ACQUIRE);
}

generates a data memory barrier defore the load:

foo:
    movw    r3, #:lower16:v
    movt    r3, #:upper16:v
    dmb     sy
    ldr     r0, [r3, #0]
    bx      lr

But, the `Acquire’ semantics imply that no reads in the current thread
dependent on the value currently loaded can be reordered before this load.
Thus, the barrier needs to be after the load and not before.

The issue seems to be in expand_atomic_load which put a memory fence before
the load in any memory model. The attached patch fixes the problem.

[Bug target/55492] __atomic_load doesn't match ACQUIRE memory model

[amacleod at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55492

Andrew Macleod <amacleod at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amacleod at redhat dot com

--- Comment #1 from Andrew Macleod <amacleod at redhat dot com> 2012-11-28 20:06:46 UTC ---
Yeah, its not right as is.

Patch is OK.

[Bug target/55492] __atomic_load doesn't match ACQUIRE memory model

[yvan.roux at linaro dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55492

--- Comment #2 from Yvan Roux <yvan.roux at linaro dot org> 2012-11-29 09:51:41 UTC ---
Thanks Andrew for the review, I can't commit the patch myself,
can you do it, or have I to post it on the mailing list ?

[Bug target/55492] __atomic_load doesn't match ACQUIRE memory model

[amacleod at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55492

--- Comment #3 from Andrew Macleod <amacleod at redhat dot com> 2012-11-29 15:20:04 UTC ---
Send it to the patches list. 

As long as you have a waiver on file allowing the code to be used I can check
it in.

[Bug target/55492] __atomic_load doesn't match ACQUIRE memory model

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55492

Richard Henderson <rth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-12-13
                 CC|                            |rth at gcc dot gnu.org
         AssignedTo|unassigned at gcc dot       |rth at gcc dot gnu.org
                   |gnu.org                     |
   Target Milestone|---                         |4.7.3
     Ever Confirmed|0                           |1

--- Comment #4 from Richard Henderson <rth at gcc dot gnu.org> 2012-12-13 21:01:32 UTC ---
Confirmed.  While the code for the above patch is good,
I'm going to update some comments at the same time.

[Bug target/55492] __atomic_load doesn't match ACQUIRE memory model

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55492

--- Comment #5 from Richard Henderson <rth at gcc dot gnu.org> 2012-12-13 21:16:50 UTC ---
Author: rth
Date: Thu Dec 13 21:16:45 2012
New Revision: 194490

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=194490
Log:
PR middle-end/55492

        * optabs.c (expand_atomic_load): Emit acquire barrier after the load.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/optabs.c

[Bug target/55492] __atomic_load doesn't match ACQUIRE memory model

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55492

--- Comment #6 from Richard Henderson <rth at gcc dot gnu.org> 2012-12-13 21:17:55 UTC ---
Author: rth
Date: Thu Dec 13 21:17:52 2012
New Revision: 194491

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=194491
Log:
PR middle-end/55492

        * optabs.c (expand_atomic_load): Emit acquire barrier after the load.

Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/optabs.c

[Bug middle-end/55492] __atomic_load doesn't match ACQUIRE memory model

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55492

Richard Henderson <rth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Status|ASSIGNED                    |RESOLVED
          Component|target                      |middle-end
         Resolution|                            |FIXED

--- Comment #7 from Richard Henderson <rth at gcc dot gnu.org> 2012-12-13 21:19:38 UTC ---
Fixed.

[Bug middle-end/55492] __atomic_load doesn't match ACQUIRE memory model

[yvan.roux at linaro dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55492

--- Comment #8 from Yvan Roux <yvan.roux at linaro dot org> 2012-12-14 10:14:15 UTC ---
Hi Richard,

Thanks for having applied and cleaned my patch.

Regarding this cleanup, if we want to be consistant I think we have to remove
the memory model test in expand_atomic_store (see the peace of patch below) as
the atomic_store valid memory model variants are RELAXED, SEQ_CST, and RELEASE
and that expand_mem_thread_fence doesn't emit any barrier in the RELEASE model
case.

Thanks,
Yvan

@@ -7537,8 +7537,7 @@ expand_atomic_store (rtx mem, rtx val, enum memmodel
model, bool use_release)
     }

   /* Otherwise assume stores are atomic, and emit the proper barriers.  */
-  if (model == MEMMODEL_SEQ_CST || model == MEMMODEL_RELEASE)
-    expand_mem_thread_fence (model);
+  expand_mem_thread_fence (model);

   emit_move_insn (mem, val);

[Bug middle-end/55492] __atomic_load doesn't match ACQUIRE memory model

[amacleod at redhat dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55492

--- Comment #9 from Andrew Macleod <amacleod at redhat dot com> 2012-12-14 15:32:52 UTC ---
Took me a second.. I presume you meant that expand_mem_thread_fence doesn't
emit anything for RELAXED mode...

Yes, since __atomic_store has already been verified by
expand_builtin_atomic_store() to only be one of the 3 valid cases, there should
not be a need for the if statement guarding the expand_mem_thread_fence.

[Bug middle-end/55492] __atomic_load doesn't match ACQUIRE memory model

[rth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55492

--- Comment #10 from Richard Henderson <rth at gcc dot gnu.org> 2012-12-14 16:29:34 UTC ---
Committed the suggested tweak, thanks.