Re: [PATCH] PR59448 - Promote consume to acquire

[Jeff Law <law@redhat.com>]

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