Re: [PATCH] PR59448 - Promote consume to acquire
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
Re: [PATCH] PR59448 - Promote consume to acquire
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
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
2015-01-14 Andrew MacLeod amacl...@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
Re: [PATCH] PR59448 - Promote consume to acquire
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 jos...@codesourcery.com
Re: [PATCH] PR59448 - Promote consume to acquire
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 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? -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
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
Re: [PATCH] PR59448 - Promote consume to acquire
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 + 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
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.
Re: [PATCH] PR59448 - Promote consume to acquire
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. Andrew
Re: [PATCH] PR59448 - Promote consume to acquire
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) Andrew
Re: [PATCH] PR59448 - Promote consume to acquire
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
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
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 jos...@codesourcery.com
Re: [PATCH] PR59448 - Promote consume to acquire
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
