[PATCH] PR59448 - Promote consume to acquire

[PATCH] PR59448 - Promote consume to acquire

[Andrew MacLeod]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

Lengthy discussion : https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59448

Basically we can generate incorrect code for an atomic consume operation 
in some circumstances.  The general feeling seems to be that we should 
simply promote all consume operations to an acquire operation until 
there is a better definition/understanding of the consume model and how 
GCC can track it.

I proposed a simple patch in the PR, and I have not seen or heard of any 
dissenting opinion.   We should get this in before the end of stage 3 I 
think.

The problem with the patch in the PR is the  memory model is immediately 
promoted from consume to acquire.   This happens *before* any of the 
memmodel checks are made.  If a consume is illegally specified (such as 
in a compare_exchange), it gets promoted to acquire and the compiler 
doesn't report the error because it never sees the consume.

This new patch simply makes the adjustment after any errors are checked 
on the originally specified model.   It bootstraps on 
x86_64-unknown-linux-gnu and passes all regression testing.
I also built an aarch64 compiler and it appears to issue the LDAR as 
specified in the PR, but anyone with a vested interest really ought to 
check it out with a real build to be sure.

OK for trunk?

Andrew

[-- Attachment #2: consume2.patch --]
[-- Type: text/x-patch, Size: 3741 bytes --]

	* builtins.c (memmodel_consume_fix) : New.  Promote consume to acquire.
	(expand_builtin_atomic_exchange, expand_builtin_atomic_compare_exchange,
	expand_builtin_atomic_load, expand_builtin_atomic_fetch_op,
	expand_builtin_atomic_clear, expand_builtin_atomic_test_and_set,
	expand_builtin_atomic_thread_fence, expand_builtin_atomic_signal_fence):
	Call memmodel_consume_fix. 

Index: builtins.c
===================================================================
*** builtins.c	(revision 219462)
--- builtins.c	(working copy)
*************** get_memmodel (tree exp)
*** 5368,5373 ****
--- 5368,5382 ----
    return (enum memmodel) val;
  }
  
+ /* Workaround for Bugzilla 59448. GCC doesn't track consume properly, so
+    be conservative and promote consume to acquire.  */
+ static void
+ memmodel_consume_fix (enum memmodel &val)
+ {
+   if (val == MEMMODEL_CONSUME)
+     val = MEMMODEL_ACQUIRE;
+ }
+ 
  /* Expand the __atomic_exchange intrinsic:
     	TYPE __atomic_exchange (TYPE *object, TYPE desired, enum memmodel)
     EXP is the CALL_EXPR.
*************** expand_builtin_atomic_exchange (machine_
*** 5389,5394 ****
--- 5398,5405 ----
    if (!flag_inline_atomics)
      return NULL_RTX;
  
+   memmodel_consume_fix (model);
+ 
    /* Expand the operands.  */
    mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
    val = expand_expr_force_mode (CALL_EXPR_ARG (exp, 1), mode);
*************** expand_builtin_atomic_compare_exchange (
*** 5434,5439 ****
--- 5445,5453 ----
    if (!flag_inline_atomics)
      return NULL_RTX;
  
+   memmodel_consume_fix (success);
+   memmodel_consume_fix (failure);
+ 
    /* Expand the operands.  */
    mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
  
*************** expand_builtin_atomic_load (machine_mode
*** 5493,5498 ****
--- 5507,5514 ----
    if (!flag_inline_atomics)
      return NULL_RTX;
  
+   memmodel_consume_fix (model);
+ 
    /* Expand the operand.  */
    mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
  
*************** expand_builtin_atomic_fetch_op (machine_
*** 5553,5558 ****
--- 5569,5576 ----
  
    model = get_memmodel (CALL_EXPR_ARG (exp, 2));
  
+   memmodel_consume_fix (model);
+ 
    /* Expand the operands.  */
    mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
    val = expand_expr_force_mode (CALL_EXPR_ARG (exp, 1), mode);
*************** expand_builtin_atomic_clear (tree exp)
*** 5627,5632 ****
--- 5645,5652 ----
        return const0_rtx;
      }
  
+   memmodel_consume_fix (model);
+ 
    if (HAVE_atomic_clear)
      {
        emit_insn (gen_atomic_clear (mem, model));
*************** expand_builtin_atomic_test_and_set (tree
*** 5658,5664 ****
    mode = mode_for_size (BOOL_TYPE_SIZE, MODE_INT, 0);
    mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
    model = get_memmodel (CALL_EXPR_ARG (exp, 1));
! 
    return expand_atomic_test_and_set (target, mem, model);
  }
  
--- 5678,5684 ----
    mode = mode_for_size (BOOL_TYPE_SIZE, MODE_INT, 0);
    mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
    model = get_memmodel (CALL_EXPR_ARG (exp, 1));
!   memmodel_consume_fix (model);
    return expand_atomic_test_and_set (target, mem, model);
  }
  
*************** static void
*** 5797,5802 ****
--- 5817,5823 ----
  expand_builtin_atomic_thread_fence (tree exp)
  {
    enum memmodel model = get_memmodel (CALL_EXPR_ARG (exp, 0));
+   memmodel_consume_fix (model);
    expand_mem_thread_fence (model);
  }
  
*************** static void
*** 5808,5813 ****
--- 5829,5835 ----
  expand_builtin_atomic_signal_fence (tree exp)
  {
    enum memmodel model = get_memmodel (CALL_EXPR_ARG (exp, 0));
+   memmodel_consume_fix (model);
    expand_mem_signal_fence (model);
  }
  

Re: [PATCH] PR59448 - Promote consume to acquire

[Richard Biener]

On Tue, Jan 13, 2015 at 3:56 PM, Andrew MacLeod <amacleod@redhat.com> wrote:
> Lengthy discussion : https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59448
>
> Basically we can generate incorrect code for an atomic consume operation in
> some circumstances.  The general feeling seems to be that we should simply
> promote all consume operations to an acquire operation until there is a
> better definition/understanding of the consume model and how GCC can track
> it.
>
> I proposed a simple patch in the PR, and I have not seen or heard of any
> dissenting opinion.   We should get this in before the end of stage 3 I
> think.
>
> The problem with the patch in the PR is the  memory model is immediately
> promoted from consume to acquire.   This happens *before* any of the
> memmodel checks are made.  If a consume is illegally specified (such as in a
> compare_exchange), it gets promoted to acquire and the compiler doesn't
> report the error because it never sees the consume.
>
> This new patch simply makes the adjustment after any errors are checked on
> the originally specified model.   It bootstraps on x86_64-unknown-linux-gnu
> and passes all regression testing.
> I also built an aarch64 compiler and it appears to issue the LDAR as
> specified in the PR, but anyone with a vested interest really ought to check
> it out with a real build to be sure.
>
> OK for trunk?

Why not patch get_memmodel?  (not sure if that catches all cases)

Richard.

>
> Andrew

Re: [PATCH] PR59448 - Promote consume to acquire

[Andrew MacLeod]

On 01/13/2015 09:59 AM, Richard Biener wrote:
> On Tue, Jan 13, 2015 at 3:56 PM, Andrew MacLeod <amacleod@redhat.com> wrote:
>> Lengthy discussion : https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59448
>>
>> Basically we can generate incorrect code for an atomic consume operation in
>> some circumstances.  The general feeling seems to be that we should simply
>> promote all consume operations to an acquire operation until there is a
>> better definition/understanding of the consume model and how GCC can track
>> it.
>>
>> I proposed a simple patch in the PR, and I have not seen or heard of any
>> dissenting opinion.   We should get this in before the end of stage 3 I
>> think.
>>
>> The problem with the patch in the PR is the  memory model is immediately
>> promoted from consume to acquire.   This happens *before* any of the
>> memmodel checks are made.  If a consume is illegally specified (such as in a
>> compare_exchange), it gets promoted to acquire and the compiler doesn't
>> report the error because it never sees the consume.
>>
>> This new patch simply makes the adjustment after any errors are checked on
>> the originally specified model.   It bootstraps on x86_64-unknown-linux-gnu
>> and passes all regression testing.
>> I also built an aarch64 compiler and it appears to issue the LDAR as
>> specified in the PR, but anyone with a vested interest really ought to check
>> it out with a real build to be sure.
>>
>> OK for trunk?
> Why not patch get_memmodel?  (not sure if that catches all cases)
>
> Richard.
>
>
That was the original patch.

The issue is that it  promotes consume to acquire before any error 
checking gets to look at the model, so then we allow illegal 
specification of consume. (It actually triggers a failure in the testsuite)

Andrew

Re: [PATCH] PR59448 - Promote consume to acquire

[Torvald Riegel]

On Tue, 2015-01-13 at 09:56 -0500, Andrew MacLeod wrote:
> The problem with the patch in the PR is the  memory model is immediately 
> promoted from consume to acquire.   This happens *before* any of the 
> memmodel checks are made.  If a consume is illegally specified (such as 
> in a compare_exchange), it gets promoted to acquire and the compiler 
> doesn't report the error because it never sees the consume.

The only issue I can think of in compare_exchange is if the program
specifies memory_order_consume for the success path but
memory_order_acquire for the failure path, which is disallowed by the
standard.

However, I don't see a reason why the standard's requirement is anything
but a performance check in our particular case.  The only case we
prevent the compiler from reporting is a consume-on-success /
acquire-on-failure combination.  But we upgrade the former to acquire,
so we can't even cause libatomic (or similar) to issue too weak barriers
due to libatomic relying on the standard's requirement.

Thus, if there's no easy way to upgrade to acquire after the sanity
checks, I think this isn't critical enough to hold up the patch from
being committed.  memory_order_consume is clearly a feature for experts.

Re: [PATCH] PR59448 - Promote consume to acquire

[Andrew MacLeod]

On 01/13/2015 10:20 AM, Torvald Riegel wrote:
> On Tue, 2015-01-13 at 09:56 -0500, Andrew MacLeod wrote:
>> The problem with the patch in the PR is the  memory model is immediately
>> promoted from consume to acquire.   This happens *before* any of the
>> memmodel checks are made.  If a consume is illegally specified (such as
>> in a compare_exchange), it gets promoted to acquire and the compiler
>> doesn't report the error because it never sees the consume.
> The only issue I can think of in compare_exchange is if the program
> specifies memory_order_consume for the success path but
> memory_order_acquire for the failure path, which is disallowed by the
> standard.
>
> However, I don't see a reason why the standard's requirement is anything
> but a performance check in our particular case.  The only case we
> prevent the compiler from reporting is a consume-on-success /
> acquire-on-failure combination.  But we upgrade the former to acquire,
> so we can't even cause libatomic (or similar) to issue too weak barriers
> due to libatomic relying on the standard's requirement.
>
> Thus, if there's no easy way to upgrade to acquire after the sanity
> checks, I think this isn't critical enough to hold up the patch from
> being committed.  memory_order_consume is clearly a feature for experts.
>
The error was actually in exchange... not compare_exchange like I wrote. 
   and causes a testsuite error that specifically tests for an illegal 
consume.

Andrew

Re: [PATCH] PR59448 - Promote consume to acquire

[Torvald Riegel]

On Tue, 2015-01-13 at 10:11 -0500, Andrew MacLeod wrote:
> On 01/13/2015 09:59 AM, Richard Biener wrote:
> > On Tue, Jan 13, 2015 at 3:56 PM, Andrew MacLeod <amacleod@redhat.com> wrote:
> >> Lengthy discussion : https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59448
> >>
> >> Basically we can generate incorrect code for an atomic consume operation in
> >> some circumstances.  The general feeling seems to be that we should simply
> >> promote all consume operations to an acquire operation until there is a
> >> better definition/understanding of the consume model and how GCC can track
> >> it.
> >>
> >> I proposed a simple patch in the PR, and I have not seen or heard of any
> >> dissenting opinion.   We should get this in before the end of stage 3 I
> >> think.
> >>
> >> The problem with the patch in the PR is the  memory model is immediately
> >> promoted from consume to acquire.   This happens *before* any of the
> >> memmodel checks are made.  If a consume is illegally specified (such as in a
> >> compare_exchange), it gets promoted to acquire and the compiler doesn't
> >> report the error because it never sees the consume.
> >>
> >> This new patch simply makes the adjustment after any errors are checked on
> >> the originally specified model.   It bootstraps on x86_64-unknown-linux-gnu
> >> and passes all regression testing.
> >> I also built an aarch64 compiler and it appears to issue the LDAR as
> >> specified in the PR, but anyone with a vested interest really ought to check
> >> it out with a real build to be sure.
> >>
> >> OK for trunk?
> > Why not patch get_memmodel?  (not sure if that catches all cases)
> >
> > Richard.
> >
> >
> That was the original patch.
> 
> The issue is that it  promotes consume to acquire before any error 
> checking gets to look at the model, so then we allow illegal 
> specification of consume. (It actually triggers a failure in the testsuite)

(This is this test: gcc/testsuite/gcc.dg/atomic-invalid.c)

The documentation of the atomic builtins also disallows mo_consume on
atomic_exchange.

However, I don't see any such requirement in C11 or C++14 (and I'd be
surprised to see it in C++11).  It would be surprising also because for
other atomic read-modify-write operations (eg, fetch_add), we don't make
such a requirement in the builtins docs -- and atomic_exchange is just a
read-modify-write with a noop, basically.

Does anyone remember why this requirement for no consume on exchange was
added, or sees a reason to keep it?  If not, I think we should drop it.
This would solve the testsuite failure for Andrew.  Dropping it would
prevent GCC from checking the consume-on-success / acquire-on-failure
case for compare_excahnge I mentioned previously, but I think that this
is pretty harmless.

I could imagine that, for some reason, either backends or libatomic do
not implement consume on atomic_exchange just because the docs
disallowed it -- but I haven't checked that.

Re: [PATCH] PR59448 - Promote consume to acquire

[Jeff Law]

On 01/13/15 11:38, Torvald Riegel wrote:
> On Tue, 2015-01-13 at 10:11 -0500, Andrew MacLeod wrote:
>> On 01/13/2015 09:59 AM, Richard Biener wrote:
>>> On Tue, Jan 13, 2015 at 3:56 PM, Andrew MacLeod <amacleod@redhat.com> wrote:
>>>> Lengthy discussion : https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59448
>>>>
>>>> Basically we can generate incorrect code for an atomic consume operation in
>>>> some circumstances.  The general feeling seems to be that we should simply
>>>> promote all consume operations to an acquire operation until there is a
>>>> better definition/understanding of the consume model and how GCC can track
>>>> it.
>>>>
>>>> I proposed a simple patch in the PR, and I have not seen or heard of any
>>>> dissenting opinion.   We should get this in before the end of stage 3 I
>>>> think.
>>>>
>>>> The problem with the patch in the PR is the  memory model is immediately
>>>> promoted from consume to acquire.   This happens *before* any of the
>>>> memmodel checks are made.  If a consume is illegally specified (such as in a
>>>> compare_exchange), it gets promoted to acquire and the compiler doesn't
>>>> report the error because it never sees the consume.
>>>>
>>>> This new patch simply makes the adjustment after any errors are checked on
>>>> the originally specified model.   It bootstraps on x86_64-unknown-linux-gnu
>>>> and passes all regression testing.
>>>> I also built an aarch64 compiler and it appears to issue the LDAR as
>>>> specified in the PR, but anyone with a vested interest really ought to check
>>>> it out with a real build to be sure.
>>>>
>>>> OK for trunk?
>>> Why not patch get_memmodel?  (not sure if that catches all cases)
>>>
>>> Richard.
>>>
>>>
>> That was the original patch.
>>
>> The issue is that it  promotes consume to acquire before any error
>> checking gets to look at the model, so then we allow illegal
>> specification of consume. (It actually triggers a failure in the testsuite)
>
> (This is this test: gcc/testsuite/gcc.dg/atomic-invalid.c)
>
> The documentation of the atomic builtins also disallows mo_consume on
> atomic_exchange.
>
> However, I don't see any such requirement in C11 or C++14 (and I'd be
> surprised to see it in C++11).  It would be surprising also because for
> other atomic read-modify-write operations (eg, fetch_add), we don't make
> such a requirement in the builtins docs -- and atomic_exchange is just a
> read-modify-write with a noop, basically.
>
> Does anyone remember why this requirement for no consume on exchange was
> added, or sees a reason to keep it?  If not, I think we should drop it.
> This would solve the testsuite failure for Andrew.  Dropping it would
> prevent GCC from checking the consume-on-success / acquire-on-failure
> case for compare_excahnge I mentioned previously, but I think that this
> is pretty harmless.
>
> I could imagine that, for some reason, either backends or libatomic do
> not implement consume on atomic_exchange just because the docs
> disallowed it -- but I haven't checked that.
AFAICT that test has been there since the initial commit of 
sync-mem-invalid.c (which was later renamed to atomic-invalid).   In 
fact, that was the only test initially in sync-mem-invalid.c
commit 64d1dbf10e3f08305f4a8569e27fc2224f9074d2
Author: amacleod <amacleod@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Thu Jun 23 13:09:31 2011 +0000

     Basica tests for __sync_mem_exchange and framework for further 
additions.

         * lib/target-support.exp (check_effective_target_sync_int_128,
         check_effective_target_sync_long_long): Check whether the target
         supports 64 and 128 bit __sync builtins.
         * gcc.dg/sync-mem.h: New. Common code to check memory model 
__syncs.
         * gcc.dg/sync-mem-1.c: New. Check char size.
         * gcc.dg/sync-mem-2.c: New. Check short size.
         * gcc.dg/sync-mem-3.c: New. Check int size.
         * gcc.dg/sync-mem-4.c: New. Check long long.
         * gcc.dg/sync-mem-5.c: New. Check 128 bit.
         * gcc.dg/sync-mem-invalid.c: New. Check invalid memory modes.



     git-svn-id: 
svn+ssh://gcc.gnu.org/svn/gcc/branches/cxx-mem-model@175331 
138bc75d-0d04-0410-961f-82ee72b054a4


Mostly hoping this refreshes Andrew's memory and he can provide some 
insight on why we test this particular combination and consider it invalid.

I was kind of hoping that we'd track this down to something like a 
particular target didn't support this capability with the old sync 
builtins and we carried it into the atomics when we made that switch.

I don't have a vested interest in either approach.  I just want to see 
us DTRT.


jeff

Re: [PATCH] PR59448 - Promote consume to acquire

[Andrew MacLeod]

On 01/13/2015 01:38 PM, Torvald Riegel wrote:
> On Tue, 2015-01-13 at 10:11 -0500, Andrew MacLeod wrote:
>> On 01/13/2015 09:59 AM, Richard Biener wrote:
>>> On Tue, Jan 13, 2015 at 3:56 PM, Andrew MacLeod <amacleod@redhat.com> wrote:
>>>> Lengthy discussion : https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59448
>>>>
>>>> Basically we can generate incorrect code for an atomic consume operation in
>>>> some circumstances.  The general feeling seems to be that we should simply
>>>> promote all consume operations to an acquire operation until there is a
>>>> better definition/understanding of the consume model and how GCC can track
>>>> it.
>>>>
>>>> I proposed a simple patch in the PR, and I have not seen or heard of any
>>>> dissenting opinion.   We should get this in before the end of stage 3 I
>>>> think.
>>>>
>>>> The problem with the patch in the PR is the  memory model is immediately
>>>> promoted from consume to acquire.   This happens *before* any of the
>>>> memmodel checks are made.  If a consume is illegally specified (such as in a
>>>> compare_exchange), it gets promoted to acquire and the compiler doesn't
>>>> report the error because it never sees the consume.
>>>>
>>>> This new patch simply makes the adjustment after any errors are checked on
>>>> the originally specified model.   It bootstraps on x86_64-unknown-linux-gnu
>>>> and passes all regression testing.
>>>> I also built an aarch64 compiler and it appears to issue the LDAR as
>>>> specified in the PR, but anyone with a vested interest really ought to check
>>>> it out with a real build to be sure.
>>>>
>>>> OK for trunk?
>>> Why not patch get_memmodel?  (not sure if that catches all cases)
>>>
>>> Richard.
>>>
>>>
>> That was the original patch.
>>
>> The issue is that it  promotes consume to acquire before any error
>> checking gets to look at the model, so then we allow illegal
>> specification of consume. (It actually triggers a failure in the testsuite)
> (This is this test: gcc/testsuite/gcc.dg/atomic-invalid.c)
>
> The documentation of the atomic builtins also disallows mo_consume on
> atomic_exchange.
>
> However, I don't see any such requirement in C11 or C++14 (and I'd be
> surprised to see it in C++11).  It would be surprising also because for
> other atomic read-modify-write operations (eg, fetch_add), we don't make
> such a requirement in the builtins docs -- and atomic_exchange is just a
> read-modify-write with a noop, basically.
>
> Does anyone remember why this requirement for no consume on exchange was
> added, or sees a reason to keep it?  If not, I think we should drop it.
> This would solve the testsuite failure for Andrew.  Dropping it would
> prevent GCC from checking the consume-on-success / acquire-on-failure
> case for compare_excahnge I mentioned previously, but I think that this
> is pretty harmless.
>
> I could imagine that, for some reason, either backends or libatomic do
> not implement consume on atomic_exchange just because the docs
> disallowed it -- but I haven't checked that.
>
I imagine it was probably in a previous incarnation of the standard...  
Most of this was actually implemented  based on very early draft 
standards years and years ago and never revised.   It wasnt put in by me 
unless the standard at some point said had such wording.   The current 
standard appears to make no mention of the situation.

It seems that it should be safe to move back to the original patch, and 
remove that error test for using consume on an exchange...

Andrew

Re: [PATCH] PR59448 - Promote consume to acquire

[Joseph Myers]

On Tue, 13 Jan 2015, Andrew MacLeod wrote:

> It seems that it should be safe to move back to the original patch, and remove
> that error test for using consume on an exchange...

I don't think there should be any such errors, for any of the atomic 
built-in functions, only warnings (with the model converted to 
MEMMODEL_SEQ_CST if not valid, just like a non-constant model).  This is 
just like any other invalid function argument of a suitable type: 
undefined behavior only if the call is executed.

http://www.open-std.org/jtc1/sc22/wg14/12553

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] PR59448 - Promote consume to acquire

[Andrew MacLeod]

On 01/13/2015 02:06 PM, Andrew MacLeod wrote:
> On 01/13/2015 01:38 PM, Torvald Riegel wrote:
>> On Tue, 2015-01-13 at 10:11 -0500, Andrew MacLeod wrote:
>>> On 01/13/2015 09:59 AM, Richard Biener wrote:
>>>> On Tue, Jan 13, 2015 at 3:56 PM, Andrew MacLeod 
>>>> <amacleod@redhat.com> wrote:
>>>>> Lengthy discussion : 
>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59448
>>>>>
>>>>> Basically we can generate incorrect code for an atomic consume 
>>>>> operation in
>>>>> some circumstances.  The general feeling seems to be that we 
>>>>> should simply
>>>>> promote all consume operations to an acquire operation until there 
>>>>> is a
>>>>> better definition/understanding of the consume model and how GCC 
>>>>> can track
>>>>> it.
>>>>>
>>>>> I proposed a simple patch in the PR, and I have not seen or heard 
>>>>> of any
>>>>> dissenting opinion.   We should get this in before the end of 
>>>>> stage 3 I
>>>>> think.
>>>>>
>>>>> The problem with the patch in the PR is the  memory model is 
>>>>> immediately
>>>>> promoted from consume to acquire.   This happens *before* any of the
>>>>> memmodel checks are made.  If a consume is illegally specified 
>>>>> (such as in a
>>>>> compare_exchange), it gets promoted to acquire and the compiler 
>>>>> doesn't
>>>>> report the error because it never sees the consume.
>>>>>
>>>>> This new patch simply makes the adjustment after any errors are 
>>>>> checked on
>>>>> the originally specified model.   It bootstraps on 
>>>>> x86_64-unknown-linux-gnu
>>>>> and passes all regression testing.
>>>>> I also built an aarch64 compiler and it appears to issue the LDAR as
>>>>> specified in the PR, but anyone with a vested interest really 
>>>>> ought to check
>>>>> it out with a real build to be sure.
>>>>>
>>>>> OK for trunk?
>>>> Why not patch get_memmodel?  (not sure if that catches all cases)
>>>>
>>>> Richard.
>>>>
>>>>
>>> That was the original patch.
>>>
>>> The issue is that it  promotes consume to acquire before any error
>>> checking gets to look at the model, so then we allow illegal
>>> specification of consume. (It actually triggers a failure in the 
>>> testsuite)
>> (This is this test: gcc/testsuite/gcc.dg/atomic-invalid.c)
>>
>> The documentation of the atomic builtins also disallows mo_consume on
>> atomic_exchange.
>>
>> However, I don't see any such requirement in C11 or C++14 (and I'd be
>> surprised to see it in C++11).  It would be surprising also because for
>> other atomic read-modify-write operations (eg, fetch_add), we don't make
>> such a requirement in the builtins docs -- and atomic_exchange is just a
>> read-modify-write with a noop, basically.
>>
>> Does anyone remember why this requirement for no consume on exchange was
>> added, or sees a reason to keep it?  If not, I think we should drop it.
>> This would solve the testsuite failure for Andrew.  Dropping it would
>> prevent GCC from checking the consume-on-success / acquire-on-failure
>> case for compare_excahnge I mentioned previously, but I think that this
>> is pretty harmless.
>>
>> I could imagine that, for some reason, either backends or libatomic do
>> not implement consume on atomic_exchange just because the docs
>> disallowed it -- but I haven't checked that.
>>
> I imagine it was probably in a previous incarnation of the 
> standard...  Most of this was actually implemented  based on very 
> early draft standards years and years ago and never revised.   It 
> wasnt put in by me unless the standard at some point said had such 
> wording.   The current standard appears to make no mention of the 
> situation.
>
> It seems that it should be safe to move back to the original patch, 
> and remove that error test for using consume on an exchange...
>
> Andrew
Here's the original patch along with the lien removed from the testcase.
x86_64-unknown-linux-gnu bootstraps, no regressions, and so forth.

OK for trunk?

Andrew

Re: [PATCH] PR59448 - Promote consume to acquire

[Jeff Law]

On 01/13/15 15:56, Andrew MacLeod wrote:
> On 01/13/2015 02:06 PM, Andrew MacLeod wrote:
>> On 01/13/2015 01:38 PM, Torvald Riegel wrote:
>>> On Tue, 2015-01-13 at 10:11 -0500, Andrew MacLeod wrote:
>>>> On 01/13/2015 09:59 AM, Richard Biener wrote:
>>>>> On Tue, Jan 13, 2015 at 3:56 PM, Andrew MacLeod
>>>>> <amacleod@redhat.com> wrote:
>>>>>> Lengthy discussion :
>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59448
>>>>>>
>>>>>> Basically we can generate incorrect code for an atomic consume
>>>>>> operation in
>>>>>> some circumstances.  The general feeling seems to be that we
>>>>>> should simply
>>>>>> promote all consume operations to an acquire operation until there
>>>>>> is a
>>>>>> better definition/understanding of the consume model and how GCC
>>>>>> can track
>>>>>> it.
>>>>>>
>>>>>> I proposed a simple patch in the PR, and I have not seen or heard
>>>>>> of any
>>>>>> dissenting opinion.   We should get this in before the end of
>>>>>> stage 3 I
>>>>>> think.
>>>>>>
>>>>>> The problem with the patch in the PR is the  memory model is
>>>>>> immediately
>>>>>> promoted from consume to acquire.   This happens *before* any of the
>>>>>> memmodel checks are made.  If a consume is illegally specified
>>>>>> (such as in a
>>>>>> compare_exchange), it gets promoted to acquire and the compiler
>>>>>> doesn't
>>>>>> report the error because it never sees the consume.
>>>>>>
>>>>>> This new patch simply makes the adjustment after any errors are
>>>>>> checked on
>>>>>> the originally specified model.   It bootstraps on
>>>>>> x86_64-unknown-linux-gnu
>>>>>> and passes all regression testing.
>>>>>> I also built an aarch64 compiler and it appears to issue the LDAR as
>>>>>> specified in the PR, but anyone with a vested interest really
>>>>>> ought to check
>>>>>> it out with a real build to be sure.
>>>>>>
>>>>>> OK for trunk?
>>>>> Why not patch get_memmodel?  (not sure if that catches all cases)
>>>>>
>>>>> Richard.
>>>>>
>>>>>
>>>> That was the original patch.
>>>>
>>>> The issue is that it  promotes consume to acquire before any error
>>>> checking gets to look at the model, so then we allow illegal
>>>> specification of consume. (It actually triggers a failure in the
>>>> testsuite)
>>> (This is this test: gcc/testsuite/gcc.dg/atomic-invalid.c)
>>>
>>> The documentation of the atomic builtins also disallows mo_consume on
>>> atomic_exchange.
>>>
>>> However, I don't see any such requirement in C11 or C++14 (and I'd be
>>> surprised to see it in C++11).  It would be surprising also because for
>>> other atomic read-modify-write operations (eg, fetch_add), we don't make
>>> such a requirement in the builtins docs -- and atomic_exchange is just a
>>> read-modify-write with a noop, basically.
>>>
>>> Does anyone remember why this requirement for no consume on exchange was
>>> added, or sees a reason to keep it?  If not, I think we should drop it.
>>> This would solve the testsuite failure for Andrew.  Dropping it would
>>> prevent GCC from checking the consume-on-success / acquire-on-failure
>>> case for compare_excahnge I mentioned previously, but I think that this
>>> is pretty harmless.
>>>
>>> I could imagine that, for some reason, either backends or libatomic do
>>> not implement consume on atomic_exchange just because the docs
>>> disallowed it -- but I haven't checked that.
>>>
>> I imagine it was probably in a previous incarnation of the
>> standard...  Most of this was actually implemented  based on very
>> early draft standards years and years ago and never revised.   It
>> wasnt put in by me unless the standard at some point said had such
>> wording.   The current standard appears to make no mention of the
>> situation.
>>
>> It seems that it should be safe to move back to the original patch,
>> and remove that error test for using consume on an exchange...
>>
>> Andrew
> Here's the original patch along with the lien removed from the testcase.
> x86_64-unknown-linux-gnu bootstraps, no regressions, and so forth.
>
> OK for trunk?
-ENOPATCH

However, I can get it from the BZ and it's OK assuming you also fixup 
the one testcase we've discussed on this thread.

Jeff

Re: [PATCH] PR59448 - Promote consume to acquire

[Andrew MacLeod]

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

On 01/13/2015 05:37 PM, Joseph Myers wrote:
> On Tue, 13 Jan 2015, Andrew MacLeod wrote:
>
>> It seems that it should be safe to move back to the original patch, and remove
>> that error test for using consume on an exchange...
> I don't think there should be any such errors, for any of the atomic
> built-in functions, only warnings (with the model converted to
> MEMMODEL_SEQ_CST if not valid, just like a non-constant model).  This is
> just like any other invalid function argument of a suitable type:
> undefined behavior only if the call is executed.
>
> http://www.open-std.org/jtc1/sc22/wg14/12553
>
Can't see what is in the link, but we've discussed this before.

- There is a warning for invalid memory models already, so I just 
continued using that.
- I remove the check for CONSUME in exchange since the current standard 
makes no mention of that being illegal.
- I also reversed the current check in compare_exchange to check for 
failure > success first, allowing us to still catch both errors if present.

I think this brings us to where we ought to be...   at least almost :-)
The latest version I have  is n3337, which still specifies that 
atomic_clear can't be memory_order_acquire or memory_order_acq_rel. Has 
that been updated to specify that memory_order_consume is not allowed 
either?  I think there was a request in at some point...   I can add 
that if so.

Bootstraps on x86_64-unknown-linux-gnu, and no regressions in the testsuite.

OK for trunk?

Andrew

[-- Attachment #2: jsmatomic.patch --]
[-- Type: text/x-patch, Size: 7052 bytes --]

2015-01-14  Andrew MacLeod  <amacleod@redhat.com>

	* builtins.c (expand_builtin_atomic_exchange): Remove error when
	memory model is CONSUME.
	(expand_builtin_atomic_compare_exchange, expand_builtin_atomic_load,
	expand_builtin_atomic_store, expand_builtin_atomic_clear): Change
	invalid memory model errors to warnings.
	* testsuite/gcc.dg/atomic-invalid.c: Check for invalid memory model
	warnings instead of errors.



Index: builtins.c
===================================================================
*** builtins.c	(revision 219601)
--- builtins.c	(working copy)
*************** expand_builtin_atomic_exchange (machine_
*** 5385,5395 ****
    enum memmodel model;
  
    model = get_memmodel (CALL_EXPR_ARG (exp, 2));
-   if ((model & MEMMODEL_MASK) == MEMMODEL_CONSUME)
-     {
-       error ("invalid memory model for %<__atomic_exchange%>");
-       return NULL_RTX;
-     }
  
    if (!flag_inline_atomics)
      return NULL_RTX;
--- 5385,5390 ----
*************** expand_builtin_atomic_compare_exchange (
*** 5422,5441 ****
    success = get_memmodel (CALL_EXPR_ARG (exp, 4));
    failure = get_memmodel (CALL_EXPR_ARG (exp, 5));
  
    if ((failure & MEMMODEL_MASK) == MEMMODEL_RELEASE
        || (failure & MEMMODEL_MASK) == MEMMODEL_ACQ_REL)
      {
!       error ("invalid failure memory model for %<__atomic_compare_exchange%>");
!       return NULL_RTX;
      }
  
!   if (failure > success)
!     {
!       error ("failure memory model cannot be stronger than success "
! 	     "memory model for %<__atomic_compare_exchange%>");
!       return NULL_RTX;
!     }
!   
    if (!flag_inline_atomics)
      return NULL_RTX;
  
--- 5417,5441 ----
    success = get_memmodel (CALL_EXPR_ARG (exp, 4));
    failure = get_memmodel (CALL_EXPR_ARG (exp, 5));
  
+   if (failure > success)
+     {
+       warning (OPT_Winvalid_memory_model,
+ 	       "failure memory model cannot be stronger than success memory "
+ 	       "model for %<__atomic_compare_exchange%>");
+       success = MEMMODEL_SEQ_CST;
+     }
+  
    if ((failure & MEMMODEL_MASK) == MEMMODEL_RELEASE
        || (failure & MEMMODEL_MASK) == MEMMODEL_ACQ_REL)
      {
!       warning (OPT_Winvalid_memory_model,
! 	       "invalid failure memory model for "
! 	       "%<__atomic_compare_exchange%>");
!       failure = MEMMODEL_SEQ_CST;
!       success = MEMMODEL_SEQ_CST;
      }
  
!  
    if (!flag_inline_atomics)
      return NULL_RTX;
  
*************** expand_builtin_atomic_load (machine_mode
*** 5491,5498 ****
    if ((model & MEMMODEL_MASK) == MEMMODEL_RELEASE
        || (model & MEMMODEL_MASK) == MEMMODEL_ACQ_REL)
      {
!       error ("invalid memory model for %<__atomic_load%>");
!       return NULL_RTX;
      }
  
    if (!flag_inline_atomics)
--- 5491,5499 ----
    if ((model & MEMMODEL_MASK) == MEMMODEL_RELEASE
        || (model & MEMMODEL_MASK) == MEMMODEL_ACQ_REL)
      {
!       warning (OPT_Winvalid_memory_model,
! 	       "invalid memory model for %<__atomic_load%>");
!       model = MEMMODEL_SEQ_CST;
      }
  
    if (!flag_inline_atomics)
*************** expand_builtin_atomic_store (machine_mod
*** 5521,5528 ****
        && (model & MEMMODEL_MASK) != MEMMODEL_SEQ_CST
        && (model & MEMMODEL_MASK) != MEMMODEL_RELEASE)
      {
!       error ("invalid memory model for %<__atomic_store%>");
!       return NULL_RTX;
      }
  
    if (!flag_inline_atomics)
--- 5522,5530 ----
        && (model & MEMMODEL_MASK) != MEMMODEL_SEQ_CST
        && (model & MEMMODEL_MASK) != MEMMODEL_RELEASE)
      {
!       warning (OPT_Winvalid_memory_model,
! 	       "invalid memory model for %<__atomic_store%>");
!       model = MEMMODEL_SEQ_CST;
      }
  
    if (!flag_inline_atomics)
*************** expand_builtin_atomic_clear (tree exp)
*** 5628,5635 ****
    if ((model & MEMMODEL_MASK) == MEMMODEL_ACQUIRE
        || (model & MEMMODEL_MASK) == MEMMODEL_ACQ_REL)
      {
!       error ("invalid memory model for %<__atomic_store%>");
!       return const0_rtx;
      }
  
    if (HAVE_atomic_clear)
--- 5630,5638 ----
    if ((model & MEMMODEL_MASK) == MEMMODEL_ACQUIRE
        || (model & MEMMODEL_MASK) == MEMMODEL_ACQ_REL)
      {
!       warning (OPT_Winvalid_memory_model,
! 	       "invalid memory model for %<__atomic_store%>");
!       model = MEMMODEL_SEQ_CST;
      }
  
    if (HAVE_atomic_clear)
Index: testsuite/gcc.dg/atomic-invalid.c
===================================================================
*** testsuite/gcc.dg/atomic-invalid.c	(revision 219601)
--- testsuite/gcc.dg/atomic-invalid.c	(working copy)
*************** bool x;
*** 13,35 ****
  int
  main ()
  {
!   __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_RELAXED, __ATOMIC_SEQ_CST); /* { dg-error "failure memory model cannot be stronger" } */
!   __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_SEQ_CST, __ATOMIC_RELEASE); /* { dg-error "invalid failure memory" } */
!   __atomic_compare_exchange_n (&i, &e, 1, 1, __ATOMIC_SEQ_CST, __ATOMIC_ACQ_REL); /* { dg-error "invalid failure memory" } */
! 
!   __atomic_load_n (&i, __ATOMIC_RELEASE); /* { dg-error "invalid memory model" } */
!   __atomic_load_n (&i, __ATOMIC_ACQ_REL); /* { dg-error "invalid memory model" } */
! 
!   __atomic_store_n (&i, 1, __ATOMIC_ACQUIRE); /* { dg-error "invalid memory model" } */
!   __atomic_store_n (&i, 1, __ATOMIC_CONSUME); /* { dg-error "invalid memory model" } */
!   __atomic_store_n (&i, 1, __ATOMIC_ACQ_REL); /* { dg-error "invalid memory model" } */
  
    i = __atomic_always_lock_free (s, NULL); /* { dg-error "non-constant argument" } */
  
    __atomic_load_n (&i, 44); /* { dg-warning "invalid memory model" } */
  
!   __atomic_clear (&x, __ATOMIC_ACQUIRE); /* { dg-error "invalid memory model" } */
  
!   __atomic_clear (&x, __ATOMIC_ACQ_REL); /* { dg-error "invalid memory model" } */
  
  }
--- 13,35 ----
  int
  main ()
  {
!   __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_RELAXED, __ATOMIC_SEQ_CST); /* { dg-warning "failure memory model cannot be stronger" } */
!   __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_SEQ_CST, __ATOMIC_RELEASE); /* { dg-warning "invalid failure memory" } */
!   __atomic_compare_exchange_n (&i, &e, 1, 1, __ATOMIC_SEQ_CST, __ATOMIC_ACQ_REL); /* { dg-warning "invalid failure memory" } */
! 
!   __atomic_load_n (&i, __ATOMIC_RELEASE); /* { dg-warning "invalid memory model" } */
!   __atomic_load_n (&i, __ATOMIC_ACQ_REL); /* { dg-warning "invalid memory model" } */
! 
!   __atomic_store_n (&i, 1, __ATOMIC_ACQUIRE); /* { dg-warning "invalid memory model" } */
!   __atomic_store_n (&i, 1, __ATOMIC_CONSUME); /* { dg-warning "invalid memory model" } */
!   __atomic_store_n (&i, 1, __ATOMIC_ACQ_REL); /* { dg-warning "invalid memory model" } */
  
    i = __atomic_always_lock_free (s, NULL); /* { dg-error "non-constant argument" } */
  
    __atomic_load_n (&i, 44); /* { dg-warning "invalid memory model" } */
  
!   __atomic_clear (&x, __ATOMIC_ACQUIRE); /* { dg-warning "invalid memory model" } */
  
!   __atomic_clear (&x, __ATOMIC_ACQ_REL); /* { dg-warning "invalid memory model" } */
  
  }

Re: [PATCH] PR59448 - Promote consume to acquire

[Torvald Riegel]

On Wed, 2015-01-14 at 10:44 -0500, Andrew MacLeod wrote:
> I think this brings us to where we ought to be...   at least almost :-)
> The latest version I have  is n3337, which still specifies that 
> atomic_clear can't be memory_order_acquire or memory_order_acq_rel. Has 
> that been updated to specify that memory_order_consume is not allowed 
> either?  I think there was a request in at some point...   I can add 
> that if so.

N3797 disallows mo_consume on atomic_flag::clear.

Re: [PATCH] PR59448 - Promote consume to acquire

[Joseph Myers]

On Wed, 14 Jan 2015, Andrew MacLeod wrote:

> - There is a warning for invalid memory models already, so I just continued
> using that.
> - I remove the check for CONSUME in exchange since the current standard makes
> no mention of that being illegal.
> - I also reversed the current check in compare_exchange to check for failure >
> success first, allowing us to still catch both errors if present.
> 
> I think this brings us to where we ought to be...   at least almost :-)
> The latest version I have  is n3337, which still specifies that atomic_clear
> can't be memory_order_acquire or memory_order_acq_rel. Has that been updated
> to specify that memory_order_consume is not allowed either?  I think there was
> a request in at some point...   I can add that if so.
> 
> Bootstraps on x86_64-unknown-linux-gnu, and no regressions in the testsuite.
> 
> OK for trunk?

OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] PR59448 - Promote consume to acquire

[Andrew MacLeod]

On 01/14/2015 01:28 PM, Joseph Myers wrote:
> On Wed, 14 Jan 2015, Andrew MacLeod wrote:
>
>> - There is a warning for invalid memory models already, so I just continued
>> using that.
>> - I remove the check for CONSUME in exchange since the current standard makes
>> no mention of that being illegal.
>> - I also reversed the current check in compare_exchange to check for failure >
>> success first, allowing us to still catch both errors if present.
>>
>> I think this brings us to where we ought to be...   at least almost :-)
>> The latest version I have  is n3337, which still specifies that atomic_clear
>> can't be memory_order_acquire or memory_order_acq_rel. Has that been updated
>> to specify that memory_order_consume is not allowed either?  I think there was
>> a request in at some point...   I can add that if so.
>>
>> Bootstraps on x86_64-unknown-linux-gnu, and no regressions in the testsuite.
>>
>> OK for trunk?
> OK.
>
checked in after disallowing memory_order_consume on atomic_clear() and 
an additional test in the testcase for that...

Andrew