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.
