This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v4, middle end]: Fix PR 37908, thinko with atomic NAND operation


Mark Mitchell wrote:

No, as explained in my previous message, it only matched wrong example.
+@emph{Note:} GCC 4.4 and later implement @code{__sync_nand_and_fetch}

__sync_fetch_and_nand, right?
We have two types of sync builtins - __sync_op_and_fetch and
__sync_fetch_and_op. This one is correct.

Attached patch revision implements suggested once-per-file warning about
changed NAND builtin semantics.

The patch wasn't attached.


However, given the explanations that have been provided, I think this is
the right thing to do, so I retract my original concern about changing
the meaning of the existing builtins. If the new note provides a hint
about how to disable the note, then the patch is OK.
Uh, my apologies... The patch is now attached.

However, I'm not sure I understood the reason to disable the note. The note can be IMO considered as "less than a warning", just a heads-up that something has been changed that requires a bit of attention.


Thanks, Uros.
Index: doc/extend.texi
===================================================================
--- doc/extend.texi	(revision 141881)
+++ doc/extend.texi	(working copy)
@@ -5774,9 +5774,12 @@
 
 @smallexample
 @{ tmp = *ptr; *ptr @var{op}= value; return tmp; @}
-@{ tmp = *ptr; *ptr = ~tmp & value; return tmp; @}   // nand
+@{ tmp = *ptr; *ptr = ~(tmp & value); return tmp; @}   // nand
 @end smallexample
 
+@emph{Note:} GCC 4.4 and later implement @code{__sync_fetch_and_nand}
+builtin as @code{*ptr = ~(tmp & value)} instead of @code{*ptr = ~tmp & value}.
+
 @item @var{type} __sync_add_and_fetch (@var{type} *ptr, @var{type} value, ...)
 @itemx @var{type} __sync_sub_and_fetch (@var{type} *ptr, @var{type} value, ...)
 @itemx @var{type} __sync_or_and_fetch (@var{type} *ptr, @var{type} value, ...)
@@ -5794,9 +5797,13 @@
 
 @smallexample
 @{ *ptr @var{op}= value; return *ptr; @}
-@{ *ptr = ~*ptr & value; return *ptr; @}   // nand
+@{ *ptr = ~(*ptr & value); return *ptr; @}   // nand
 @end smallexample
 
+@emph{Note:} GCC 4.4 and later implement @code{__sync_nand_and_fetch}
+builtin as @code{*ptr = ~(*ptr & value)} instead of
+@code{*ptr = ~*ptr & value}.
+
 @item bool __sync_bool_compare_and_swap (@var{type} *ptr, @var{type} oldval @var{type} newval, ...)
 @itemx @var{type} __sync_val_compare_and_swap (@var{type} *ptr, @var{type} oldval @var{type} newval, ...)
 @findex __sync_bool_compare_and_swap
Index: optabs.c
===================================================================
--- optabs.c	(revision 141881)
+++ optabs.c	(working copy)
@@ -7182,12 +7182,13 @@
       t1 = t0;
       if (code == NOT)
 	{
-	  t1 = expand_simple_unop (mode, NOT, t1, NULL_RTX, true);
-	  code = AND;
+	  t1 = expand_simple_binop (mode, AND, t1, val, NULL_RTX,
+				    true, OPTAB_LIB_WIDEN);
+	  t1 = expand_simple_unop (mode, code, t1, NULL_RTX, true);
 	}
-      t1 = expand_simple_binop (mode, code, t1, val, NULL_RTX,
-				true, OPTAB_LIB_WIDEN);
-
+      else
+	t1 = expand_simple_binop (mode, code, t1, val, NULL_RTX,
+				  true, OPTAB_LIB_WIDEN);
       insn = get_insns ();
       end_sequence ();
 
@@ -7315,9 +7316,17 @@
 		}
 
 	      if (code == NOT)
-		target = expand_simple_unop (mode, NOT, target, NULL_RTX, true);
-	      target = expand_simple_binop (mode, code, target, val, NULL_RTX,
-					    true, OPTAB_LIB_WIDEN);
+		{
+		  target = expand_simple_binop (mode, AND, target, val,
+						NULL_RTX, true,
+						OPTAB_LIB_WIDEN);
+		  target = expand_simple_unop (mode, code, target,
+					       NULL_RTX, true);
+		}
+	      else
+		target = expand_simple_binop (mode, code, target, val,
+					      NULL_RTX, true,
+					      OPTAB_LIB_WIDEN);
 	    }
 
 	  return target;
@@ -7340,11 +7349,13 @@
       t1 = t0;
       if (code == NOT)
 	{
-	  t1 = expand_simple_unop (mode, NOT, t1, NULL_RTX, true);
-	  code = AND;
+	  t1 = expand_simple_binop (mode, AND, t1, val, NULL_RTX,
+				    true, OPTAB_LIB_WIDEN);
+	  t1 = expand_simple_unop (mode, code, t1, NULL_RTX, true);
 	}
-      t1 = expand_simple_binop (mode, code, t1, val, NULL_RTX,
-				true, OPTAB_LIB_WIDEN);
+      else
+	t1 = expand_simple_binop (mode, code, t1, val, NULL_RTX,
+				  true, OPTAB_LIB_WIDEN);
       if (after)
 	emit_move_insn (target, t1);
 
Index: builtins.c
===================================================================
--- builtins.c	(revision 141881)
+++ builtins.c	(working copy)
@@ -5988,6 +5988,50 @@
   rtx val, mem;
   enum machine_mode old_mode;
 
+  if (code == NOT)
+    {
+      tree fndecl = get_callee_fndecl (exp);
+      enum built_in_function fcode = DECL_FUNCTION_CODE (fndecl);
+
+      static bool warned_f_a_n, warned_n_a_f;
+
+      switch (fcode)
+	{
+	case BUILT_IN_FETCH_AND_NAND_1:
+	case BUILT_IN_FETCH_AND_NAND_2:
+	case BUILT_IN_FETCH_AND_NAND_4:
+	case BUILT_IN_FETCH_AND_NAND_8:
+	case BUILT_IN_FETCH_AND_NAND_16:
+
+	  if (warned_f_a_n)
+	    break;
+
+	  fndecl = implicit_built_in_decls[BUILT_IN_FETCH_AND_NAND_N];
+	  inform (input_location,
+		  "%qD changed semantics in GCC 4.4", fndecl);
+	  warned_f_a_n = true;
+	  break;
+
+	case BUILT_IN_NAND_AND_FETCH_1:
+	case BUILT_IN_NAND_AND_FETCH_2:
+	case BUILT_IN_NAND_AND_FETCH_4:
+	case BUILT_IN_NAND_AND_FETCH_8:
+	case BUILT_IN_NAND_AND_FETCH_16:
+
+	  if (warned_n_a_f)
+	    break;
+
+	  fndecl = implicit_built_in_decls[BUILT_IN_NAND_AND_FETCH_N];
+	  inform (input_location,
+		  "%qD changed semantics in GCC 4.4", fndecl);
+	  warned_n_a_f = true;
+	  break;
+
+	default:
+	  gcc_unreachable ();
+	}
+    }
+
   /* Expand the operands.  */
   mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
 
Index: testsuite/gcc.c-torture/compile/sync-1.c
===================================================================
--- testsuite/gcc.c-torture/compile/sync-1.c	(revision 141881)
+++ testsuite/gcc.c-torture/compile/sync-1.c	(working copy)
@@ -1,3 +1,6 @@
+/* { dg-message "note: '__sync_fetch_and_nand' changed semantics in GCC 4.4" "" { target *-*-* } 0 } */
+/* { dg-message "note: '__sync_nand_and_fetch' changed semantics in GCC 4.4" "" { target *-*-* } 0 } */
+
 /* Validate that each of the __sync builtins compiles.  This won't 
    necessarily link, since the target might not support the builtin,
    so this may result in external library calls.  */
Index: testsuite/gcc.c-torture/compile/sync-2.c
===================================================================
--- testsuite/gcc.c-torture/compile/sync-2.c	(revision 141881)
+++ testsuite/gcc.c-torture/compile/sync-2.c	(working copy)
@@ -1,3 +1,5 @@
+/* { dg-message "note: '__sync_fetch_and_nand' changed semantics in GCC 4.4" "" { target *-*-* } 0 } */
+
 /* Validate that each of the __sync builtins compiles.  This won't 
    necessarily link, since the target might not support the builtin,
    so this may result in external library calls.  */
Index: testsuite/gcc.c-torture/compile/sync-3.c
===================================================================
--- testsuite/gcc.c-torture/compile/sync-3.c	(revision 141881)
+++ testsuite/gcc.c-torture/compile/sync-3.c	(working copy)
@@ -1,3 +1,5 @@
+/* { dg-message "note: '__sync_fetch_and_nand' changed semantics in GCC 4.4" "" { target *-*-* } 0 } */
+
 /* Validate that each of the __sync builtins compiles.  This won't 
    necessarily link, since the target might not support the builtin,
    so this may result in external library calls.  */
Index: testsuite/gcc.dg/ia64-sync-2.c
===================================================================
--- testsuite/gcc.dg/ia64-sync-2.c	(revision 141881)
+++ testsuite/gcc.dg/ia64-sync-2.c	(working copy)
@@ -4,6 +4,9 @@
 /* { dg-options "-march=i486" { target { { i?86-*-* x86_64-*-* } && ilp32 } } } */
 /* { dg-options "-mcpu=v9" { target sparc*-*-* } } */
 
+/* { dg-message "note: '__sync_fetch_and_nand' changed semantics in GCC 4.4" "" { target *-*-* } 0 } */
+/* { dg-message "note: '__sync_nand_and_fetch' changed semantics in GCC 4.4" "" { target *-*-* } 0 } */
+
 /* Test basic functionality of the intrinsics.  */
 
 __extension__ typedef __SIZE_TYPE__ size_t;
@@ -12,8 +15,8 @@
 extern void *memcpy (void *, const void *, size_t);
 
 static int AI[18];
-static int init_si[18] = { 0,0,0,1,0,0,0,0,-1,0,0,0,0,0,-1,0,0,0 };
-static int test_si[18] = { 1,1,1,1,1,4,22,-12,7,8,9,7,1,-12,7,8,9,7 };
+static int init_si[18] = { 0,0,0,1,0,0, 0,0  ,-1,0,0,-1,0,0  ,-1,0,0,-1 };
+static int test_si[18] = { 1,1,1,1,1,4,22,-12,7 ,8,9,~7,1,-12,7 ,8,9,~7 };
 
 static void
 do_si (void)
@@ -44,7 +47,7 @@
     abort ();
   if (__sync_fetch_and_xor(AI+10, 9) != 0)
     abort ();
-  if (__sync_fetch_and_nand(AI+11, 7) != 0)
+  if (__sync_fetch_and_nand(AI+11, 7) != -1)
     abort ();
 
   if (__sync_add_and_fetch(AI+12, 1) != 1)
@@ -57,13 +60,13 @@
     abort ();
   if (__sync_xor_and_fetch(AI+16, 9) != 9)
     abort ();
-  if (__sync_nand_and_fetch(AI+17, 7) != 7)
+  if (__sync_nand_and_fetch(AI+17, 7) != ~7)
     abort ();
 }
 
 static long AL[18];
-static long init_di[18] = { 0,0,0,1,0,0,0,0,-1,0,0,0,0,0,-1,0,0,0 };
-static long test_di[18] = { 1,1,1,1,1,4,22,-12,7,8,9,7,1,-12,7,8,9,7 };
+static long init_di[18] = { 0,0,0,1,0,0, 0,0  ,-1,0,0,-1,0,0  ,-1,0,0,-1 };
+static long test_di[18] = { 1,1,1,1,1,4,22,-12,7 ,8,9,~7,1,-12,7 ,8,9,~7 };
 
 static void
 do_di (void)
@@ -94,7 +97,7 @@
     abort ();
   if (__sync_fetch_and_xor(AL+10, 9) != 0)
     abort ();
-  if (__sync_fetch_and_nand(AL+11, 7) != 0)
+  if (__sync_fetch_and_nand(AL+11, 7) != -1)
     abort ();
 
   if (__sync_add_and_fetch(AL+12, 1) != 1)
@@ -107,7 +110,7 @@
     abort ();
   if (__sync_xor_and_fetch(AL+16, 9) != 9)
     abort ();
-  if (__sync_nand_and_fetch(AL+17, 7) != 7)
+  if (__sync_nand_and_fetch(AL+17, 7) != ~7)
     abort ();
 }
 
Index: testsuite/gcc.dg/sync-3.c
===================================================================
--- testsuite/gcc.dg/sync-3.c	(revision 141881)
+++ testsuite/gcc.dg/sync-3.c	(working copy)
@@ -1,10 +1,102 @@
 /* { dg-do run } */
 /* { dg-require-effective-target sync_char_short } */
-/* { dg-options "-O2" } */
-/* { dg-options "-march=i486 -O2" { target { { i?86-*-* x86_64-*-* } && ilp32 } } } */
-/* { dg-options "-mcpu=v9 -O2" { target sparc*-*-* } } */
+/* { dg-options "-ansi" } */
+/* { dg-options "-march=i486" { target { { i?86-*-* x86_64-*-* } && ilp32 } } } */
+/* { dg-options "-mcpu=v9" { target sparc*-*-* } } */
 
 /* Test functionality of the intrinsics for 'short' and 'char'.  */
 
-#define AI_ALIGN __attribute__((__aligned__ (4)))
-#include "sync-2.c"
+extern void abort (void);
+extern void *memcpy (void *, const void *, __SIZE_TYPE__);
+
+static char AI[18] __attribute__((__aligned__ (4)));
+static char init_qi[18] = { 3,5,7,9,0,0,0 ,0  ,-1,0,0,-1,0,0  ,-1,0,0,-1 };
+static char test_qi[18] = { 3,5,7,9,1,4,22,-12,7 ,8,9,~7,1,-12,7 ,8,9,~7 };
+
+static void
+do_qi (void)
+{
+  if (__sync_fetch_and_add(AI+4, 1) != 0)
+    abort ();
+  if (__sync_fetch_and_add(AI+5, 4) != 0)
+    abort ();
+  if (__sync_fetch_and_add(AI+6, 22) != 0)
+    abort ();
+  if (__sync_fetch_and_sub(AI+7, 12) != 0)
+    abort ();
+  if (__sync_fetch_and_and(AI+8, 7) != (char)-1)
+    abort ();
+  if (__sync_fetch_and_or(AI+9, 8) != 0)
+    abort ();
+  if (__sync_fetch_and_xor(AI+10, 9) != 0)
+    abort ();
+  if (__sync_fetch_and_nand(AI+11, 7) != (char)-1) /* { dg-message "note: '__sync_fetch_and_nand' changed semantics in GCC 4.4" "" } */
+    abort ();
+
+  if (__sync_add_and_fetch(AI+12, 1) != 1)
+    abort ();
+  if (__sync_sub_and_fetch(AI+13, 12) != (char)-12)
+    abort ();
+  if (__sync_and_and_fetch(AI+14, 7) != 7)
+    abort ();
+  if (__sync_or_and_fetch(AI+15, 8) != 8)
+    abort ();
+  if (__sync_xor_and_fetch(AI+16, 9) != 9)
+    abort ();
+  if (__sync_nand_and_fetch(AI+17, 7) != ~7) /* { dg-message "note: '__sync_nand_and_fetch' changed semantics in GCC 4.4" "" } */
+    abort ();
+}
+
+static short AL[18];
+static short init_hi[18] = { 3,5,7,9,0,0,0 ,0  ,-1,0,0,-1,0,0  ,-1,0,0,-1 };
+static short test_hi[18] = { 3,5,7,9,1,4,22,-12,7 ,8,9,~7,1,-12,7 ,8,9,~7 };
+
+static void
+do_hi (void)
+{
+  if (__sync_fetch_and_add(AL+4, 1) != 0)
+    abort ();
+  if (__sync_fetch_and_add(AL+5, 4) != 0)
+    abort ();
+  if (__sync_fetch_and_add(AL+6, 22) != 0)
+    abort ();
+  if (__sync_fetch_and_sub(AL+7, 12) != 0)
+    abort ();
+  if (__sync_fetch_and_and(AL+8, 7) != -1)
+    abort ();
+  if (__sync_fetch_and_or(AL+9, 8) != 0)
+    abort ();
+  if (__sync_fetch_and_xor(AL+10, 9) != 0)
+    abort ();
+  if (__sync_fetch_and_nand(AL+11, 7) != -1)
+    abort ();
+
+  if (__sync_add_and_fetch(AL+12, 1) != 1)
+    abort ();
+  if (__sync_sub_and_fetch(AL+13, 12) != -12)
+    abort ();
+  if (__sync_and_and_fetch(AL+14, 7) != 7)
+    abort ();
+  if (__sync_or_and_fetch(AL+15, 8) != 8)
+    abort ();
+  if (__sync_xor_and_fetch(AL+16, 9) != 9)
+    abort ();
+  if (__sync_nand_and_fetch(AL+17, 7) != ~7)
+    abort ();
+}
+
+int main()
+{
+  memcpy(AI, init_qi, sizeof(init_qi));
+  memcpy(AL, init_hi, sizeof(init_hi));
+
+  do_qi ();
+  do_hi ();
+
+  if (memcmp (AI, test_qi, sizeof(test_qi)))
+    abort ();
+  if (memcmp (AL, test_hi, sizeof(test_hi)))
+    abort ();
+
+  return 0;
+}
Index: testsuite/gcc.dg/ia64-sync-1.c
===================================================================
--- testsuite/gcc.dg/ia64-sync-1.c	(revision 141881)
+++ testsuite/gcc.dg/ia64-sync-1.c	(working copy)
@@ -4,6 +4,8 @@
 /* { dg-options "-march=i486" { target { { i?86-*-* x86_64-*-* } && ilp32 } } } */
 /* { dg-options "-mcpu=v9" { target sparc*-*-* } } */
 
+/* { dg-message "note: '__sync_fetch_and_nand' changed semantics in GCC 4.4" "" { target *-*-* } 0 } */
+
 /* Test basic functionality of the intrinsics.  The operations should
    not be optimized away if no one checks the return values.  */
 
@@ -13,8 +15,8 @@
 extern void *memcpy (void *, const void *, size_t);
 
 static int AI[12];
-static int init_noret_si[12] = { 0, 0, 0, 1, 0, 0, 0, 0, -1, 0, 0, 0 };
-static int test_noret_si[12] = { 1, 1, 1, 0, 1, 4, 22, -12, 7, 8, 9, 7 };
+static int init_noret_si[12] = { 0, 0, 0, 1, 0, 0, 0 , 0  , -1, 0, 0, -1 };
+static int test_noret_si[12] = { 1, 1, 1, 0, 1, 4, 22, -12, 7 , 8, 9, ~7 };
 
 static void
 do_noret_si (void)
@@ -35,8 +37,8 @@
 }
 
 static long AL[12];
-static long init_noret_di[12] = { 0, 0, 0, 1, 0, 0, 0, 0, -1, 0, 0, 0 };
-static long test_noret_di[12] = { 1, 1, 1, 0, 1, 4, 22, -12, 7, 8, 9, 7 };
+static long init_noret_di[12] = { 0, 0, 0, 1, 0, 0, 0 , 0  , -1, 0, 0, -1 };
+static long test_noret_di[12] = { 1, 1, 1, 0, 1, 4, 22, -12, 7 , 8, 9, ~7 };
 
 static void
 do_noret_di (void)
Index: testsuite/gcc.dg/sync-2.c
===================================================================
--- testsuite/gcc.dg/sync-2.c	(revision 141881)
+++ testsuite/gcc.dg/sync-2.c	(working copy)
@@ -4,19 +4,18 @@
 /* { dg-options "-march=i486" { target { { i?86-*-* x86_64-*-* } && ilp32 } } } */
 /* { dg-options "-mcpu=v9" { target sparc*-*-* } } */
 
+/* { dg-message "note: '__sync_fetch_and_nand' changed semantics in GCC 4.4" "" { target *-*-* } 0 } */
+/* { dg-message "note: '__sync_nand_and_fetch' changed semantics in GCC 4.4" "" { target *-*-* } 0 } */
+
 /* Test functionality of the intrinsics for 'short' and 'char'.  */
 
 extern void abort (void);
 extern void *memcpy (void *, const void *, __SIZE_TYPE__);
 
-#ifndef AI_ALIGN
-#define AI_ALIGN
-#endif
+static char AI[18];
+static char init_qi[18] = { 3,5,7,9,0,0,0 ,0  ,-1,0,0,-1,0,0  ,-1,0,0,-1 };
+static char test_qi[18] = { 3,5,7,9,1,4,22,-12,7 ,8,9,~7,1,-12,7 ,8,9,~7 };
 
-static char AI[18] AI_ALIGN;
-static char init_qi[18] = { 3,5,7,9,0,0,0,0,-1,0,0,0,0,0,-1,0,0,0 };
-static char test_qi[18] = { 3,5,7,9,1,4,22,-12,7,8,9,7,1,-12,7,8,9,7 };
-
 static void
 do_qi (void)
 {
@@ -34,7 +33,7 @@
     abort ();
   if (__sync_fetch_and_xor(AI+10, 9) != 0)
     abort ();
-  if (__sync_fetch_and_nand(AI+11, 7) != 0)
+  if (__sync_fetch_and_nand(AI+11, 7) != (char)-1)
     abort ();
 
   if (__sync_add_and_fetch(AI+12, 1) != 1)
@@ -47,13 +46,13 @@
     abort ();
   if (__sync_xor_and_fetch(AI+16, 9) != 9)
     abort ();
-  if (__sync_nand_and_fetch(AI+17, 7) != 7)
+  if (__sync_nand_and_fetch(AI+17, 7) != ~7)
     abort ();
 }
 
 static short AL[18];
-static short init_hi[18] = { 3,5,7,9,0,0,0,0,-1,0,0,0,0,0,-1,0,0,0 };
-static short test_hi[18] = { 3,5,7,9,1,4,22,-12,7,8,9,7,1,-12,7,8,9,7 };
+static short init_hi[18] = { 3,5,7,9,0,0,0 ,0  ,-1,0,0,-1,0,0  ,-1,0,0,-1 };
+static short test_hi[18] = { 3,5,7,9,1,4,22,-12,7 ,8,9,~7,1,-12,7 ,8,9,~7 };
 
 static void
 do_hi (void)
@@ -72,7 +71,7 @@
     abort ();
   if (__sync_fetch_and_xor(AL+10, 9) != 0)
     abort ();
-  if (__sync_fetch_and_nand(AL+11, 7) != 0)
+  if (__sync_fetch_and_nand(AL+11, 7) != -1)
     abort ();
 
   if (__sync_add_and_fetch(AL+12, 1) != 1)
@@ -85,7 +84,7 @@
     abort ();
   if (__sync_xor_and_fetch(AL+16, 9) != 9)
     abort ();
-  if (__sync_nand_and_fetch(AL+17, 7) != 7)
+  if (__sync_nand_and_fetch(AL+17, 7) != ~7)
     abort ();
 }
 

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]