diff mbox

PR59448 - Promote consume to acquire

Message ID 54B68ECD.5070409@redhat.com
State New
Headers show

Commit Message

Andrew MacLeod Jan. 14, 2015, 3:44 p.m. UTC 
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

Comments

Torvald Riegel Jan. 14, 2015, 3:50 p.m. UTC | #1 
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.
Joseph Myers Jan. 14, 2015, 6:28 p.m. UTC | #2 
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.
Andrew MacLeod Jan. 14, 2015, 9:31 p.m. UTC | #3 
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
diff mbox

Patch

2015-01-14  Andrew MacLeod  <amacleod@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 for %<__atomic_store%>");
!       model = MEMMODEL_SEQ_CST;
      }
  
    if (!flag_inline_atomics)
*************** expand_builtin_atomic_clear (tree exp)
*** 5628,5635 ****
    if ((model & MEMMODEL_MASK) == MEMMODEL_ACQUIRE
        || (model & MEMMODEL_MASK) == MEMMODEL_ACQ_REL)
      {
!       error ("invalid memory model for %<__atomic_store%>");
!       return const0_rtx;
      }
  
    if (HAVE_atomic_clear)
--- 5630,5638 ----
    if ((model & MEMMODEL_MASK) == MEMMODEL_ACQUIRE
        || (model & MEMMODEL_MASK) == MEMMODEL_ACQ_REL)
      {
!       warning (OPT_Winvalid_memory_model,
! 	       "invalid memory model for %<__atomic_store%>");
!       model = MEMMODEL_SEQ_CST;
      }
  
    if (HAVE_atomic_clear)
Index: testsuite/gcc.dg/atomic-invalid.c
===================================================================
*** testsuite/gcc.dg/atomic-invalid.c	(revision 219601)
--- testsuite/gcc.dg/atomic-invalid.c	(working copy)
*************** bool x;
*** 13,35 ****
  int
  main ()
  {
!   __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_RELAXED, __ATOMIC_SEQ_CST); /* { dg-error "failure memory model cannot be stronger" } */
!   __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_SEQ_CST, __ATOMIC_RELEASE); /* { dg-error "invalid failure memory" } */
!   __atomic_compare_exchange_n (&i, &e, 1, 1, __ATOMIC_SEQ_CST, __ATOMIC_ACQ_REL); /* { dg-error "invalid failure memory" } */
! 
!   __atomic_load_n (&i, __ATOMIC_RELEASE); /* { dg-error "invalid memory model" } */
!   __atomic_load_n (&i, __ATOMIC_ACQ_REL); /* { dg-error "invalid memory model" } */
! 
!   __atomic_store_n (&i, 1, __ATOMIC_ACQUIRE); /* { dg-error "invalid memory model" } */
!   __atomic_store_n (&i, 1, __ATOMIC_CONSUME); /* { dg-error "invalid memory model" } */
!   __atomic_store_n (&i, 1, __ATOMIC_ACQ_REL); /* { dg-error "invalid memory model" } */
  
    i = __atomic_always_lock_free (s, NULL); /* { dg-error "non-constant argument" } */
  
    __atomic_load_n (&i, 44); /* { dg-warning "invalid memory model" } */
  
!   __atomic_clear (&x, __ATOMIC_ACQUIRE); /* { dg-error "invalid memory model" } */
  
!   __atomic_clear (&x, __ATOMIC_ACQ_REL); /* { dg-error "invalid memory model" } */
  
  }
--- 13,35 ----
  int
  main ()
  {
!   __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_RELAXED, __ATOMIC_SEQ_CST); /* { dg-warning "failure memory model cannot be stronger" } */
!   __atomic_compare_exchange_n (&i, &e, 1, 0, __ATOMIC_SEQ_CST, __ATOMIC_RELEASE); /* { dg-warning "invalid failure memory" } */
!   __atomic_compare_exchange_n (&i, &e, 1, 1, __ATOMIC_SEQ_CST, __ATOMIC_ACQ_REL); /* { dg-warning "invalid failure memory" } */
! 
!   __atomic_load_n (&i, __ATOMIC_RELEASE); /* { dg-warning "invalid memory model" } */
!   __atomic_load_n (&i, __ATOMIC_ACQ_REL); /* { dg-warning "invalid memory model" } */
! 
!   __atomic_store_n (&i, 1, __ATOMIC_ACQUIRE); /* { dg-warning "invalid memory model" } */
!   __atomic_store_n (&i, 1, __ATOMIC_CONSUME); /* { dg-warning "invalid memory model" } */
!   __atomic_store_n (&i, 1, __ATOMIC_ACQ_REL); /* { dg-warning "invalid memory model" } */
  
    i = __atomic_always_lock_free (s, NULL); /* { dg-error "non-constant argument" } */
  
    __atomic_load_n (&i, 44); /* { dg-warning "invalid memory model" } */
  
!   __atomic_clear (&x, __ATOMIC_ACQUIRE); /* { dg-warning "invalid memory model" } */
  
!   __atomic_clear (&x, __ATOMIC_ACQ_REL); /* { dg-warning "invalid memory model" } */
  
  }