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