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
<amacl...@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
