[Patch] 1/2 Expand sync_sub as sync_add if predicates don't match.

[Patch] 1/2 Expand sync_sub as sync_add if predicates don't match.

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

While implementing the atomic memory operations for MIPS, I came across 
a problem where immediate operands were forced into a register when 
sync_sub* insns were expanded.  On MIPS there is no minus instruction 
for immediate operands, instead this should be expanded as plus with the 
negative value of the immediate operand.

In expand_sync_operation() and expand_sync_fetch_operation() there is 
code to handle conversion to plus if the minus insn does not exits.  I 
enhanced this slightly so that it also does the conversion if the 
predicate for sync_sub* does not match, but it does match for the 
corresponding sync_add*.  This allows me to write sync_sub* insns with a 
register predicate and have them used for non-immediate operands, but an 
immediate operand results in conversion to sync_add*.

Tested on x86_64-pc-linux-gnu all default languages both native and -m32 
with no regressions.  Also tested mips64-linux C only.

:ADDPATCH middle-end:

OK to commit?

2007-08-20  David Daney  <ddaney@avtrex.com>

    * optabs.c (expand_sync_operation):  Use plus insn if minus insn
    predicate does not match val.
    (expand_sync_fetch_operation):  Ditto.


[-- Attachment #2: optabs.diff --]
[-- Type: text/x-patch, Size: 2688 bytes --]

Index: optabs.c
===================================================================
--- optabs.c	(revision 127356)
+++ optabs.c	(working copy)
@@ -6420,7 +6420,7 @@ rtx
 expand_sync_operation (rtx mem, rtx val, enum rtx_code code)
 {
   enum machine_mode mode = GET_MODE (mem);
-  enum insn_code icode;
+  enum insn_code icode, plus_icode;
   rtx insn;
 
   /* Look to see if the target supports the operation directly.  */
@@ -6444,9 +6444,17 @@ expand_sync_operation (rtx mem, rtx val,
 
     case MINUS:
       icode = sync_sub_optab[mode];
-      if (icode == CODE_FOR_nothing)
+      plus_icode = sync_add_optab[mode];
+
+      /* Change sign and add either if there is no insn for MINUS, or
+         if the MINUS insn cannot directly handle VAL and the PLUS
+         insn can.  */
+      if (icode == CODE_FOR_nothing
+          || (!insn_data[icode].operand[1].predicate (val, mode)
+              && plus_icode != CODE_FOR_nothing
+              && insn_data[plus_icode].operand[1].predicate (val, mode)))
 	{
-	  icode = sync_add_optab[mode];
+          icode = plus_icode;
 	  if (icode != CODE_FOR_nothing)
 	    {
 	      val = expand_simple_unop (mode, NEG, val, NULL_RTX, 1);
@@ -6513,7 +6521,7 @@ expand_sync_fetch_operation (rtx mem, rt
 			     bool after, rtx target)
 {
   enum machine_mode mode = GET_MODE (mem);
-  enum insn_code old_code, new_code, icode;
+  enum insn_code old_code, new_code, plus_old_code, plus_new_code, icode;
   bool compensate;
   rtx insn;
 
@@ -6544,10 +6552,20 @@ expand_sync_fetch_operation (rtx mem, rt
     case MINUS:
       old_code = sync_old_sub_optab[mode];
       new_code = sync_new_sub_optab[mode];
-      if (old_code == CODE_FOR_nothing && new_code == CODE_FOR_nothing)
+      plus_old_code = sync_old_add_optab[mode];
+      plus_new_code = sync_new_add_optab[mode];
+
+      /* Change sign and add either if there are no insns for MINUS, or
+         if the MINUS insns cannot directly handle VAL and the PLUS
+         insns can.  */
+      if ((old_code == CODE_FOR_nothing && new_code == CODE_FOR_nothing)
+          || (after && !insn_data[new_code].operand[1].predicate (val, mode)
+              && insn_data[plus_new_code].operand[1].predicate (val, mode))
+          || (!after && !insn_data[old_code].operand[1].predicate (val, mode)
+              && insn_data[plus_old_code].operand[1].predicate (val, mode)))
 	{
-	  old_code = sync_old_add_optab[mode];
-	  new_code = sync_new_add_optab[mode];
+          old_code = plus_old_code;
+          new_code = plus_new_code;
 	  if (old_code != CODE_FOR_nothing || new_code != CODE_FOR_nothing)
 	    {
 	      val = expand_simple_unop (mode, NEG, val, NULL_RTX, 1);

Re: [Patch] 2/2 MIPS atomic memory operations.

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 1807 bytes --]

This patch adds atomic memory operation for MIPS.  It depends on part 1 
of the patch which is a change to the generic expanders.

In:
http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01128.html

Richard essentially pre-approved this, but I wanted to post it again 
because of how it interacts with the changes I want to make to 
expand_sync_operation() and expand_sync_fetch_operation().

Tested on mips64-linux.

OK to commit if the first part is approved?

gcc/

2007-08-20  David Daney  <ddaney@avtrex.com>

    * config/mips/mips.md (UNSPEC_COMPARE_AND_SWAP, UNSPEC_SYNC_OLD_OP,
    UNSPEC_SYNC_NEW_OP, UNSPEC_SYNC_EXCHANGE): New define_constants.
    (optab, insn): Add more attributes.
    (fetchop_bit): New code macro.
    (immediate_insn): New code macro attribute.
    (sync): Change condition to ISA_HAS_SYNC.
    (rdhwr): Change predicate for operand 0 to register_operand.
    (memory_barrier): New expand.
    (sync_compare_and_swap<mode>, sync_add<mode>, sync_sub<mode>,
    sync_old_add<mode>, sync_old_sub<mode>, sync_new_add<mode>,
    sync_new_sub<mode>, sync_<optab><mode>,    sync_old_<optab><mode>,
    sync_new_<optab><mode>, sync_nand<mode>, sync_old_nand<mode>,
    sync_new_nand<mode>, sync_lock_test_and_set<mode>): New insns.
    * config/mips/mips.h (ISA_HAS_SYNC, ISA_HAS_LL_SC): New ISA predicates.
    (MIPS_COMPARE_AND_SWAP, MIPS_SYNC_OP, MIPS_SYNC_OLD_OP,
    MIPS_SYNC_NEW_OP, MIPS_SYNC_NAND, MIPS_SYNC_OLD_NAND,
    MIPS_SYNC_NEW_NAND, MIPS_SYNC_EXCHANGE): New Macros.
   

gcc/testsuite/
2007-08-20  David Daney  <ddaney@avtrex.com>
    * gcc.target/mips/gcc-have-sync-compare-and-swap-1.c: New test.
    * gcc.target/mips/gcc-have-sync-compare-and-swap-2.c: Ditto.
    * gcc.target/mips/atomic-memory-1.c: Ditto.
    * testsuite/gcc.target/mips/atomic-memory-2.c: Ditto.


[-- Attachment #2: mips-atomic.diff --]
[-- Type: text/x-patch, Size: 16900 bytes --]

Index: config/mips/mips.md
===================================================================
--- config/mips/mips.md	(revision 127356)
+++ config/mips/mips.md	(working copy)
@@ -53,7 +53,11 @@ (define_constants
    (UNSPEC_RDHWR		34)
    (UNSPEC_SYNCI		35)
    (UNSPEC_SYNC			36)
-
+   (UNSPEC_COMPARE_AND_SWAP	37)
+   (UNSPEC_SYNC_OLD_OP		38)
+   (UNSPEC_SYNC_NEW_OP		39)
+   (UNSPEC_SYNC_EXCHANGE	40)
+   
    (UNSPEC_ADDRESS_FIRST	100)
 
    (FAKE_CALL_REGNO		79)
@@ -576,12 +580,18 @@ (define_code_attr su [(sign_extend "s") 
 ;; <optab> expands to the name of the optab for a particular code.
 (define_code_attr optab [(ashift "ashl")
 			 (ashiftrt "ashr")
-			 (lshiftrt "lshr")])
+			 (lshiftrt "lshr")
+			 (ior "ior")
+			 (xor "xor")
+			 (and "and")])
 
 ;; <insn> expands to the name of the insn that implements a particular code.
 (define_code_attr insn [(ashift "sll")
 			(ashiftrt "sra")
-			(lshiftrt "srl")])
+			(lshiftrt "srl")
+			(ior "or")
+			(xor "xor")
+			(and "and")])
 
 ;; <fcond> is the c.cond.fmt condition associated with a particular code.
 (define_code_attr fcond [(unordered "un")
@@ -597,6 +607,14 @@ (define_code_attr swapped_fcond [(ge "le
 				 (gt "lt")
 				 (unge "ule")
 				 (ungt "ult")])
+
+;; Atomic fetch bitwise operations.
+(define_code_macro fetchop_bit [ior xor and])
+
+;; <immediate_insn> expands to the name of the insn that implements
+;; a particular code to operate in immediate values.
+(define_code_attr immediate_insn [(ior "ori") (xor "xori") (and "andi")])
+
 \f
 ;; .........................
 ;;
@@ -4251,7 +4269,7 @@ (define_expand "clear_cache"
 
 (define_insn "sync"
   [(unspec_volatile [(const_int 0)] UNSPEC_SYNC)]
-  "ISA_HAS_SYNCI"
+  "ISA_HAS_SYNC"
   "sync")
 
 (define_insn "synci"
@@ -4261,7 +4279,7 @@ (define_insn "synci"
   "synci\t0(%0)")
 
 (define_insn "rdhwr"
-  [(set (match_operand:SI 0 "general_operand" "=d")
+  [(set (match_operand:SI 0 "register_operand" "=d")
         (unspec_volatile [(match_operand:SI 1 "const_int_operand" "n")]
         UNSPEC_RDHWR))]
   "ISA_HAS_SYNCI"
@@ -4283,6 +4301,225 @@ (define_insn "clear_hazard"
          "\t.set\tpop";
 }
   [(set_attr "length" "20")])
+
+;; Atomic memory operations.
+
+(define_expand "memory_barrier"
+  [(unspec_volatile [(const_int 0)] UNSPEC_SYNC)]
+  "ISA_HAS_SYNC"
+  "")
+
+(define_insn "sync_compare_and_swap<mode>"
+  [(set (match_operand:GPR 0 "register_operand" "=&d,d")
+	(match_operand:GPR 1 "memory_operand" "+R,R"))
+   (set (match_dup 1)
+	(unspec_volatile:GPR [(match_operand:GPR 2 "register_operand" "d,d")
+			      (match_operand:GPR 3 "arith_operand" "I,d")]
+	 UNSPEC_COMPARE_AND_SWAP))]
+  "ISA_HAS_LL_SC"
+{
+  if (which_alternative == 0)
+    return MIPS_COMPARE_AND_SWAP ("<d>", "li");
+  else
+    return MIPS_COMPARE_AND_SWAP ("<d>", "move");
+}
+  [(set_attr "length" "28")])
+
+(define_insn "sync_add<mode>"
+  [(set (match_operand:GPR 0 "memory_operand" "+R,R")
+	(unspec_volatile:GPR
+          [(plus:GPR (match_dup 0)
+			      (match_operand:GPR 1 "arith_operand" "I,d"))]
+	 UNSPEC_SYNC_OLD_OP))]
+  "ISA_HAS_LL_SC"
+{
+  if (which_alternative == 0)
+    return MIPS_SYNC_OP ("<d>", "<d>addiu");	
+  else
+    return MIPS_SYNC_OP ("<d>", "<d>addu");	
+}
+  [(set_attr "length" "24")])
+
+(define_insn "sync_sub<mode>"
+  [(set (match_operand:GPR 0 "memory_operand" "+R")
+	(unspec_volatile:GPR
+          [(minus:GPR (match_dup 0)
+			      (match_operand:GPR 1 "register_operand" "d"))]
+	 UNSPEC_SYNC_OLD_OP))]
+  "ISA_HAS_LL_SC"
+{
+    return MIPS_SYNC_OP ("<d>", "<d>subu");	
+}
+  [(set_attr "length" "24")])
+
+(define_insn "sync_old_add<mode>"
+  [(set (match_operand:GPR 0 "register_operand" "=&d,d")
+	(match_operand:GPR 1 "memory_operand" "+R,R"))
+   (set (match_dup 1)
+	(unspec_volatile:GPR
+          [(plus:GPR (match_dup 1)
+		     (match_operand:GPR 2 "arith_operand" "I,d"))]
+	 UNSPEC_SYNC_OLD_OP))]
+  "ISA_HAS_LL_SC"
+{
+  if (which_alternative == 0)
+    return MIPS_SYNC_OLD_OP ("<d>", "<d>addiu");	
+  else
+    return MIPS_SYNC_OLD_OP ("<d>", "<d>addu");	
+}
+  [(set_attr "length" "24")])
+
+(define_insn "sync_old_sub<mode>"
+  [(set (match_operand:GPR 0 "register_operand" "=&d")
+	(match_operand:GPR 1 "memory_operand" "+R"))
+   (set (match_dup 1)
+	(unspec_volatile:GPR
+          [(minus:GPR (match_dup 1)
+		      (match_operand:GPR 2 "register_operand" "d"))]
+	 UNSPEC_SYNC_OLD_OP))]
+  "ISA_HAS_LL_SC"
+{
+  return MIPS_SYNC_OLD_OP ("<d>", "<d>subu");	
+}
+  [(set_attr "length" "24")])
+
+(define_insn "sync_new_add<mode>"
+  [(set (match_operand:GPR 0 "register_operand" "=&d,d")
+        (plus:GPR (match_operand:GPR 1 "memory_operand" "+R,R")
+		  (match_operand:GPR 2 "arith_operand" "I,d")))
+   (set (match_dup 1)
+	(unspec_volatile:GPR
+	  [(plus:GPR (match_dup 1) (match_dup 2))]
+	 UNSPEC_SYNC_NEW_OP))]
+  "ISA_HAS_LL_SC"
+{
+  if (which_alternative == 0)
+    return MIPS_SYNC_NEW_OP ("<d>", "<d>addiu");	
+  else
+    return MIPS_SYNC_NEW_OP ("<d>", "<d>addu");	
+}
+  [(set_attr "length" "24")])
+
+(define_insn "sync_new_sub<mode>"
+  [(set (match_operand:GPR 0 "register_operand" "=&d")
+        (minus:GPR (match_operand:GPR 1 "memory_operand" "+R")
+		   (match_operand:GPR 2 "register_operand" "d")))
+   (set (match_dup 1)
+	(unspec_volatile:GPR
+	  [(minus:GPR (match_dup 1) (match_dup 2))]
+	 UNSPEC_SYNC_NEW_OP))]
+  "ISA_HAS_LL_SC"
+{
+  return MIPS_SYNC_NEW_OP ("<d>", "<d>subu");	
+}
+  [(set_attr "length" "24")])
+
+(define_insn "sync_<optab><mode>"
+  [(set (match_operand:GPR 0 "memory_operand" "+R,R")
+	(unspec_volatile:GPR
+          [(fetchop_bit:GPR (match_operand:GPR 1 "uns_arith_operand" "K,d")
+			      (match_dup 0))]
+	 UNSPEC_SYNC_OLD_OP))]
+  "ISA_HAS_LL_SC"
+{
+  if (which_alternative == 0)
+    return MIPS_SYNC_OP ("<d>", "<immediate_insn>");	
+  else
+    return MIPS_SYNC_OP ("<d>", "<insn>");	
+}
+  [(set_attr "length" "24")])
+
+(define_insn "sync_old_<optab><mode>"
+  [(set (match_operand:GPR 0 "register_operand" "=&d,d")
+	(match_operand:GPR 1 "memory_operand" "+R,R"))
+   (set (match_dup 1)
+	(unspec_volatile:GPR
+          [(fetchop_bit:GPR (match_operand:GPR 2 "uns_arith_operand" "K,d")
+			    (match_dup 1))]
+	 UNSPEC_SYNC_OLD_OP))]
+  "ISA_HAS_LL_SC"
+{
+  if (which_alternative == 0)
+    return MIPS_SYNC_OLD_OP ("<d>", "<immediate_insn>");	
+  else
+    return MIPS_SYNC_OLD_OP ("<d>", "<insn>");	
+}
+  [(set_attr "length" "24")])
+
+(define_insn "sync_new_<optab><mode>"
+  [(set (match_operand:GPR 0 "register_operand" "=&d,d")
+	(match_operand:GPR 1 "memory_operand" "+R,R"))
+   (set (match_dup 1)
+	(unspec_volatile:GPR
+          [(fetchop_bit:GPR (match_operand:GPR 2 "uns_arith_operand" "K,d")
+			    (match_dup 1))]
+	 UNSPEC_SYNC_NEW_OP))]
+  "ISA_HAS_LL_SC"
+{
+  if (which_alternative == 0)
+    return MIPS_SYNC_NEW_OP ("<d>", "<immediate_insn>");	
+  else
+    return MIPS_SYNC_NEW_OP ("<d>", "<insn>");	
+}
+  [(set_attr "length" "24")])
+
+(define_insn "sync_nand<mode>"
+  [(set (match_operand:GPR 0 "memory_operand" "+R,R")
+	(unspec_volatile:GPR [(match_operand:GPR 1 "uns_arith_operand" "K,d")]
+	 UNSPEC_SYNC_OLD_OP))]
+  "ISA_HAS_LL_SC"
+{
+  if (which_alternative == 0)
+    return MIPS_SYNC_NAND ("<d>", "andi");	
+  else
+    return MIPS_SYNC_NAND ("<d>", "and");	
+}
+  [(set_attr "length" "28")])
+
+(define_insn "sync_old_nand<mode>"
+  [(set (match_operand:GPR 0 "register_operand" "=&d,d")
+	(match_operand:GPR 1 "memory_operand" "+R,R"))
+   (set (match_dup 1)
+        (unspec_volatile:GPR [(match_operand:GPR 2 "uns_arith_operand" "K,d")]
+	 UNSPEC_SYNC_OLD_OP))]
+  "ISA_HAS_LL_SC"
+{
+  if (which_alternative == 0)
+    return MIPS_SYNC_OLD_NAND ("<d>", "andi");	
+  else
+    return MIPS_SYNC_OLD_NAND ("<d>", "and");	
+}
+  [(set_attr "length" "28")])
+
+(define_insn "sync_new_nand<mode>"
+  [(set (match_operand:GPR 0 "register_operand" "=&d,d")
+	(match_operand:GPR 1 "memory_operand" "+R,R"))
+   (set (match_dup 1)
+	(unspec_volatile:GPR [(match_operand:GPR 2 "uns_arith_operand" "K,d")]
+	 UNSPEC_SYNC_NEW_OP))]
+  "ISA_HAS_LL_SC"
+{
+  if (which_alternative == 0)
+    return MIPS_SYNC_NEW_NAND ("<d>", "andi");	
+  else
+    return MIPS_SYNC_NEW_NAND ("<d>", "and");	
+}
+  [(set_attr "length" "28")])
+
+(define_insn "sync_lock_test_and_set<mode>"
+  [(set (match_operand:GPR 0 "register_operand" "=&d,d")
+	(match_operand:GPR 1 "memory_operand" "+R,R"))
+   (set (match_dup 1)
+	(unspec_volatile:GPR [(match_operand:GPR 2 "arith_operand" "I,d")]
+	 UNSPEC_SYNC_EXCHANGE))]
+  "ISA_HAS_LL_SC"
+{
+  if (which_alternative == 0)
+    return MIPS_SYNC_EXCHANGE ("<d>", "li");
+  else
+    return MIPS_SYNC_EXCHANGE ("<d>", "move");
+}
+  [(set_attr "length" "24")])
 \f
 ;; Block moves, see mips.c for more details.
 ;; Argument 0 is the destination
Index: config/mips/mips.h
===================================================================
--- config/mips/mips.h	(revision 127356)
+++ config/mips/mips.h	(working copy)
@@ -876,6 +876,13 @@ extern enum mips_code_readable_setting m
 /* ISA includes synci, jr.hb and jalr.hb.  */
 #define ISA_HAS_SYNCI (ISA_MIPS32R2 && !TARGET_MIPS16)
 
+/* ISA includes sync.  */
+#define ISA_HAS_SYNC ((mips_isa >= 2 || TARGET_MIPS3900) && !TARGET_MIPS16)
+
+/* ISA includes ll and sc.  Note that this implies ISA_HAS_SYNC
+   because the expanders use both ISA_HAS_SYNC and ISA_HAS_LL_SC
+   instructions.  */
+#define ISA_HAS_LL_SC (mips_isa >= 2 && !TARGET_MIPS16)
 \f
 /* Add -G xx support.  */
 
@@ -2815,3 +2822,140 @@ while (0)
 #ifndef HAVE_AS_TLS
 #define HAVE_AS_TLS 0
 #endif
+
+/* Return an asm string that atomically:
+
+     - Compares memory reference %1 to register %2 and, if they are
+       equal, changes %1 to %3.
+
+     - Sets register %0 to the old value of memory reference %1.
+
+   SUFFIX is the suffix that should be added to "ll" and "sc" instructions
+   and OP is the instruction that should be used to load %3 into a
+   register.  */
+#define MIPS_COMPARE_AND_SWAP(SUFFIX, OP)	\
+  "%(%<%[sync\n"				\
+  "1:\tll" SUFFIX "\t%0,%1\n"			\
+  "\tbne\t%0,%2,2f\n"				\
+  "\t" OP "\t%@,%3\n"				\
+  "\tsc" SUFFIX "\t%@,%1\n"			\
+  "\tbeq\t%@,%.,1b\n"				\
+  "\tnop\n"					\
+  "2:%]%>%)"
+
+/* Return an asm string that atomically:
+
+     - Sets memory reference %0 to %0 INSN %1.
+
+   SUFFIX is the suffix that should be added to "ll" and "sc"
+   instructions.  */
+#define MIPS_SYNC_OP(SUFFIX, INSN)		\
+  "%(%<%[sync\n"				\
+  "1:\tll" SUFFIX "\t%@,%0\n"			\
+  "\t" INSN "\t%@,%@,%1\n"			\
+  "\tsc" SUFFIX "\t%@,%0\n"			\
+  "\tbeq\t%@,%.,1b\n"				\
+  "\tnop%]%>%)"
+
+/* Return an asm string that atomically:
+
+     - Sets memory reference %1 to %1 INSN %2.
+
+     - Sets register %0 to the old value of memory reference %1.
+
+   SUFFIX is the suffix that should be added to "ll" and "sc"
+   instructions.  */
+#define MIPS_SYNC_OLD_OP(SUFFIX, INSN)		\
+  "%(%<%[sync\n"				\
+  "1:\tll" SUFFIX "\t%0,%1\n"			\
+  "\t" INSN "\t%@,%0,%2\n"			\
+  "\tsc" SUFFIX "\t%@,%1\n"			\
+  "\tbeq\t%@,%.,1b\n"				\
+  "\tnop%]%>%)"
+
+/* Return an asm string that atomically:
+
+     - Sets memory reference %1 to %1 INSN %2.
+
+     - Sets register %0 to the new value of memory reference %1.
+
+   SUFFIX is the suffix that should be added to "ll" and "sc"
+   instructions.  */
+#define MIPS_SYNC_NEW_OP(SUFFIX, INSN)		\
+  "%(%<%[sync\n"				\
+  "1:\tll" SUFFIX "\t%0,%1\n"			\
+  "\t" INSN "\t%@,%0,%2\n"			\
+  "\tsc" SUFFIX "\t%@,%1\n"			\
+  "\tbeq\t%@,%.,1b\n"				\
+  "\t" INSN "\t%0,%0,%2%]%>%)"
+
+/* Return an asm string that atomically:
+
+     - Sets memory reference %0 to ~%0 AND %1.
+
+   SUFFIX is the suffix that should be added to "ll" and "sc"
+   instructions.  INSN is the and instruction needed to and a register
+   with %2.  */
+#define MIPS_SYNC_NAND(SUFFIX, INSN)		\
+  "%(%<%[sync\n"				\
+  "1:\tll" SUFFIX "\t%@,%0\n"			\
+  "\tnor\t%@,%@,%.\n"				\
+  "\t" INSN "\t%@,%@,%1\n"			\
+  "\tsc" SUFFIX "\t%@,%0\n"			\
+  "\tbeq\t%@,%.,1b\n"				\
+  "\tnop%]%>%)"
+
+/* Return an asm string that atomically:
+
+     - Sets memory reference %1 to ~%1 AND %2.
+
+     - Sets register %0 to the old value of memory reference %1.
+
+   SUFFIX is the suffix that should be added to "ll" and "sc"
+   instructions.  INSN is the and instruction needed to and a register
+   with %2.  */
+#define MIPS_SYNC_OLD_NAND(SUFFIX, INSN)	\
+  "%(%<%[sync\n"				\
+  "1:\tll" SUFFIX "\t%0,%1\n"			\
+  "\tnor\t%@,%0,%.\n"				\
+  "\t" INSN "\t%@,%@,%2\n"			\
+  "\tsc" SUFFIX "\t%@,%1\n"			\
+  "\tbeq\t%@,%.,1b\n"				\
+  "\tnop%]%>%)"
+
+/* Return an asm string that atomically:
+
+     - Sets memory reference %1 to ~%1 AND %2.
+
+     - Sets register %0 to the new value of memory reference %1.
+
+   SUFFIX is the suffix that should be added to "ll" and "sc"
+   instructions.  INSN is the and instruction needed to and a register
+   with %2.  */
+#define MIPS_SYNC_NEW_NAND(SUFFIX, INSN)	\
+  "%(%<%[sync\n"				\
+  "1:\tll" SUFFIX "\t%0,%1\n"			\
+  "\tnor\t%0,%0,%.\n"				\
+  "\t" INSN "\t%@,%0,%2\n"			\
+  "\tsc" SUFFIX "\t%@,%1\n"			\
+  "\tbeq\t%@,%.,1b\n"				\
+  "\t" INSN "\t%0,%0,%2%]%>%)"
+
+/* Return an asm string that atomically:
+
+     - Sets memory reference %1 to %2.
+
+     - Sets register %0 to the old value of memory reference %1.
+
+   SUFFIX is the suffix that should be added to "ll" and "sc"
+   instructions.  OP is the and instruction that should be used to
+   load %2 into a register.  */
+#define MIPS_SYNC_EXCHANGE(SUFFIX, OP)		\
+  "%(%<%[\n"					\
+  "1:\tll" SUFFIX "\t%0,%1\n"			\
+  "\t" OP "\t%@,%2\n"				\
+  "\tsc" SUFFIX "\t%@,%1\n"			\
+  "\tbeq\t%@,%.,1b\n"				\
+  "\tnop\n"					\
+  "\tsync%]%>%)"
+
Index: testsuite/gcc.target/mips/gcc-have-sync-compare-and-swap-1.c
===================================================================
--- testsuite/gcc.target/mips/gcc-have-sync-compare-and-swap-1.c	(revision 0)
+++ testsuite/gcc.target/mips/gcc-have-sync-compare-and-swap-1.c	(revision 0)
@@ -0,0 +1,22 @@
+/* { dg-do preprocess } */
+/* { dg-options "-mips32 -mabi=32" } */
+
+#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1
+#error nonono
+#endif
+
+#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2
+#error nonono
+#endif
+
+#ifndef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
+#error nonono
+#endif
+
+#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8
+#error nonono
+#endif
+
+#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16
+#error nonono
+#endif
Index: testsuite/gcc.target/mips/atomic-memory-2.c
===================================================================
--- testsuite/gcc.target/mips/atomic-memory-2.c	(revision 0)
+++ testsuite/gcc.target/mips/atomic-memory-2.c	(revision 0)
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-mips-options "-O2 -mips32 -mabi=32" } */
+/* { dg-final { scan-assembler "addiu" } } */
+/* { dg-final { scan-assembler-not "subu" } } */
+
+unsigned long
+f(unsigned long *p)
+{
+    return __sync_fetch_and_sub (p, 5);
+}
Index: testsuite/gcc.target/mips/atomic-memory-1.c
===================================================================
--- testsuite/gcc.target/mips/atomic-memory-1.c	(revision 0)
+++ testsuite/gcc.target/mips/atomic-memory-1.c	(revision 0)
@@ -0,0 +1,41 @@
+/* { dg-do run } */
+extern void abort (void);
+extern void exit (int);
+
+int main ()
+{
+#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
+  unsigned v = 0;
+  __sync_synchronize ();
+
+  if (!__sync_bool_compare_and_swap (&v, 0, 30000))
+    abort();
+  if (30000 != __sync_val_compare_and_swap (&v, 30000, 100001))
+    abort();
+  __sync_sub_and_fetch (&v, 0x8001);
+  __sync_sub_and_fetch (&v, 0x7fff);
+  if (v != 34465)
+    abort();
+  if (__sync_nand_and_fetch (&v, 0xff) != 94)
+    abort();
+  if (__sync_fetch_and_add (&v, 6) != 94)
+    abort();
+  if (v != 100)
+    abort();
+  if (__sync_or_and_fetch (&v, 0xf001) != 0xf065)
+    abort();
+  if (__sync_and_and_fetch (&v, 0x1000) != 0x1000)
+    abort();
+  if (__sync_xor_and_fetch (&v, 0xa51040) != 0xa50040)
+    abort();
+  __sync_and_and_fetch (&v, 7);
+  if (__sync_lock_test_and_set(&v, 1) != 0)
+    abort();
+  if (v != 1)
+    abort();
+  __sync_lock_release (&v);
+  if (v != 0)
+    abort();
+#endif
+  exit(0);
+}
Index: testsuite/gcc.target/mips/gcc-have-sync-compare-and-swap-2.c
===================================================================
--- testsuite/gcc.target/mips/gcc-have-sync-compare-and-swap-2.c	(revision 0)
+++ testsuite/gcc.target/mips/gcc-have-sync-compare-and-swap-2.c	(revision 0)
@@ -0,0 +1,22 @@
+/* { dg-do preprocess { target { mips64*-*-* } } } */
+/* { dg-options "-mips64 -mabi=64" } */
+
+#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1
+#error nonono
+#endif
+
+#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2
+#error nonono
+#endif
+
+#ifndef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4
+#error nonono
+#endif
+
+#ifndef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8
+#error nonono
+#endif
+
+#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16
+#error nonono
+#endif

Re: [Patch] 2/2 MIPS atomic memory operations.

[Richard Sandiford]

David Daney <ddaney@avtrex.com> writes:
> This patch adds atomic memory operation for MIPS.  It depends on part 1 
> of the patch which is a change to the generic expanders.
>
> In:
> http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01128.html
>
> Richard essentially pre-approved this, but I wanted to post it again 
> because of how it interacts with the changes I want to make to 
> expand_sync_operation() and expand_sync_fetch_operation().
>
> Tested on mips64-linux.
>
> OK to commit if the first part is approved?

Yes, thanks.  And thanks once again for your patience.

Richard

Re: [Patch] 2/2 MIPS atomic memory operations.

[David Daney]

Richard Sandiford wrote:
> David Daney <ddaney@avtrex.com> writes:
>   
>> This patch adds atomic memory operation for MIPS.  It depends on part 1 
>> of the patch which is a change to the generic expanders.
>>
>> In:
>> http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01128.html
>>
>> Richard essentially pre-approved this, but I wanted to post it again 
>> because of how it interacts with the changes I want to make to 
>> expand_sync_operation() and expand_sync_fetch_operation().
>>
>> Tested on mips64-linux.
>>
>> OK to commit if the first part is approved?
>>     
>
> Yes, thanks.  And thanks once again for your patience.
>   
On the somewhat winding path I took to arrive at this patch, I never 
tested it without part 1/2.  However after getting a bit restless 
waiting for part 1/2 to be approved, I thought I would try it without 
part 1/2.  It turns out that the first part of the patch is not strictly 
speaking necessary, so I went ahead and committed this.  The lreg pass 
appears to be transforming:

(insn 6 2 7 2 st3.c:17 (set (reg:SI 194)
        (const_int -1 [0xffffffffffffffff])) 211 {*movsi_internal} (nil))

(insn 7 6 15 2 st3.c:17 (set (mem/v:SI (reg/v/f:SI 193 [ pv ]) [-1 S4 A32])
        (unspec_volatile:SI [
                (minus:SI (mem/v:SI (reg/v/f:SI 193 [ pv ]) [-1 S4 A32])
                    (reg:SI 194))
            ] 38)) 261 {sync_subsi} (expr_list:REG_DEAD (reg:SI 194)
        (expr_list:REG_DEAD (reg/v/f:SI 193 [ pv ])
            (nil))))

Into:
(insn 7 2 15 2 st3.c:17 (set (mem/v:SI (reg/v/f:SI 193 [ pv ]) [-1 S4 A32])
        (unspec_volatile:SI [
                (plus:SI (mem/v:SI (reg/v/f:SI 193 [ pv ]) [-1 S4 A32])
                    (const_int 1 [0x1]))
            ] 38)) 259 {sync_addsi} (expr_list:REG_DEAD (reg/v/f:SI 193 
[ pv ])
        (nil)))

It still may be a good idea to expand it this way in the first place (as 
part 1 of the patch did).  I don't really know.

David Daney

Re: [Patch] 2/2 MIPS atomic memory operations.

[Richard Sandiford]

David Daney <ddaney@avtrex.com> writes:
> Richard Sandiford wrote:
>> David Daney <ddaney@avtrex.com> writes:
>>   
>>> This patch adds atomic memory operation for MIPS.  It depends on part 1 
>>> of the patch which is a change to the generic expanders.
>>>
>>> In:
>>> http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01128.html
>>>
>>> Richard essentially pre-approved this, but I wanted to post it again 
>>> because of how it interacts with the changes I want to make to 
>>> expand_sync_operation() and expand_sync_fetch_operation().
>>>
>>> Tested on mips64-linux.
>>>
>>> OK to commit if the first part is approved?
>>>     
>>
>> Yes, thanks.  And thanks once again for your patience.
>>   
> On the somewhat winding path I took to arrive at this patch, I never 
> tested it without part 1/2.  However after getting a bit restless 
> waiting for part 1/2 to be approved, I thought I would try it without 
> part 1/2.  It turns out that the first part of the patch is not strictly 
> speaking necessary, so I went ahead and committed this.

Thanks.

> The lreg pass appears to be transforming:
>
> (insn 6 2 7 2 st3.c:17 (set (reg:SI 194)
>         (const_int -1 [0xffffffffffffffff])) 211 {*movsi_internal} (nil))
>
> (insn 7 6 15 2 st3.c:17 (set (mem/v:SI (reg/v/f:SI 193 [ pv ]) [-1 S4 A32])
>         (unspec_volatile:SI [
>                 (minus:SI (mem/v:SI (reg/v/f:SI 193 [ pv ]) [-1 S4 A32])
>                     (reg:SI 194))
>             ] 38)) 261 {sync_subsi} (expr_list:REG_DEAD (reg:SI 194)
>         (expr_list:REG_DEAD (reg/v/f:SI 193 [ pv ])
>             (nil))))
>
> Into:
> (insn 7 2 15 2 st3.c:17 (set (mem/v:SI (reg/v/f:SI 193 [ pv ]) [-1 S4 A32])
>         (unspec_volatile:SI [
>                 (plus:SI (mem/v:SI (reg/v/f:SI 193 [ pv ]) [-1 S4 A32])
>                     (const_int 1 [0x1]))
>             ] 38)) 259 {sync_addsi} (expr_list:REG_DEAD (reg/v/f:SI 193 
> [ pv ])
>         (nil)))
>
> It still may be a good idea to expand it this way in the first place (as 
> part 1 of the patch did).  I don't really know.

FWIW, I think part 1 should still go in.  I'm disappointed that it has
taken so long for you to get a review of something that's fixing an
obvious source of noncanonical rtl.

Richard

Re: [Patch] 2/2 MIPS atomic memory operations.

[Maxim Kuvyrkov]

David Daney wrote:
> This patch adds atomic memory operation for MIPS.  It depends on part 1 
> of the patch which is a change to the generic expanders.
> 
> In:
> http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01128.html
> 
> Richard essentially pre-approved this, but I wanted to post it again 
> because of how it interacts with the changes I want to make to 
> expand_sync_operation() and expand_sync_fetch_operation().
> 
> Tested on mips64-linux.
> 
> OK to commit if the first part is approved?
> 
> gcc/
> 
> 2007-08-20  David Daney  <ddaney@avtrex.com>
> 
>    * config/mips/mips.md (UNSPEC_COMPARE_AND_SWAP, UNSPEC_SYNC_OLD_OP,
>    UNSPEC_SYNC_NEW_OP, UNSPEC_SYNC_EXCHANGE): New define_constants.
>    (optab, insn): Add more attributes.
>    (fetchop_bit): New code macro.

Hi David,

Recently define_{mode, code}_macro were ranamed to define_{mode, 
code}_iterator.  Hence your patch broke mips on the trunk.  Can you 
please fix it?


Thanks,

Maxim

Re: [Patch] 2/2 MIPS atomic memory operations.

[Richard Sandiford]

Maxim Kuvyrkov <maxim@codesourcery.com> writes:
> David Daney wrote:
>> This patch adds atomic memory operation for MIPS.  It depends on part 1 
>> of the patch which is a change to the generic expanders.
>> 
>> In:
>> http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01128.html
>> 
>> Richard essentially pre-approved this, but I wanted to post it again 
>> because of how it interacts with the changes I want to make to 
>> expand_sync_operation() and expand_sync_fetch_operation().
>> 
>> Tested on mips64-linux.
>> 
>> OK to commit if the first part is approved?
>> 
>> gcc/
>> 
>> 2007-08-20  David Daney  <ddaney@avtrex.com>
>> 
>>    * config/mips/mips.md (UNSPEC_COMPARE_AND_SWAP, UNSPEC_SYNC_OLD_OP,
>>    UNSPEC_SYNC_NEW_OP, UNSPEC_SYNC_EXCHANGE): New define_constants.
>>    (optab, insn): Add more attributes.
>>    (fetchop_bit): New code macro.
>
> Hi David,
>
> Recently define_{mode, code}_macro were ranamed to define_{mode, 
> code}_iterator.  Hence your patch broke mips on the trunk.  Can you 
> please fix it?

Thanks for the heads-up.  I've committed this fix on David's behalf.
For future reference, it's perfectly OK (indeed quite helpful ;))
for the first person who finds something like this to commit
the fix as obvious.

Richard


gcc/
	* config/mips/mips.md (fetchop_bit): Use define_code_iterator
	rather than define_code_macro.

Index: gcc/config/mips/mips.md
===================================================================
--- gcc/config/mips/mips.md	(revision 128038)
+++ gcc/config/mips/mips.md	(working copy)
@@ -609,7 +609,7 @@ (define_code_attr swapped_fcond [(ge "le
 				 (ungt "ult")])
 
 ;; Atomic fetch bitwise operations.
-(define_code_macro fetchop_bit [ior xor and])
+(define_code_iterator fetchop_bit [ior xor and])
 
 ;; <immediate_insn> expands to the name of the insn that implements
 ;; a particular code to operate in immediate values.

Re: [Patch] 2/2 MIPS atomic memory operations.

[David Daney]

David Daney wrote:> +
> +(define_expand "memory_barrier"
> +  [(unspec_volatile [(const_int 0)] UNSPEC_SYNC)]
> +  "ISA_HAS_SYNC"
> +  "")

Uh, I think "memory_barrier" must have a memory clobber.

This is probably the cause of the FAIL for libjava.lang/PR27908.java

I will try to fix it in a follow-up patch.

David Daney

Re: [Patch] 1/2 Expand sync_sub as sync_add if predicates don't match.

[Richard Sandiford]

David Daney <ddaney@avtrex.com> writes:
> While implementing the atomic memory operations for MIPS, I came across 
> a problem where immediate operands were forced into a register when 
> sync_sub* insns were expanded.  On MIPS there is no minus instruction 
> for immediate operands, instead this should be expanded as plus with the 
> negative value of the immediate operand.
>
> In expand_sync_operation() and expand_sync_fetch_operation() there is 
> code to handle conversion to plus if the minus insn does not exits.  I 
> enhanced this slightly so that it also does the conversion if the 
> predicate for sync_sub* does not match, but it does match for the 
> corresponding sync_add*.  This allows me to write sync_sub* insns with a 
> register predicate and have them used for non-immediate operands, but an 
> immediate operand results in conversion to sync_add*.
>
> Tested on x86_64-pc-linux-gnu all default languages both native and -m32 
> with no regressions.  Also tested mips64-linux C only.
>
> :ADDPATCH middle-end:
>
> OK to commit?

FWIW, with (minus ... (const_int ...)) not being canonical rtl, I think
we should always use PLUS rather than MINUS if the operand is a CONST_INT.
E.g. the first thing expand_binop does is:

  /* If subtracting an integer constant, convert this into an addition of
     the negated constant.  */
  if (binoptab == sub_optab && GET_CODE (op1) == CONST_INT)
    {
      op1 = negate_rtx (mode, op1);
      binoptab = add_optab;
    }

and I think we should have a similar thing in expand_sync_operation
and expand_sync_fetch_operation.  As well as avoiding invalid
expressions, it has the advantage of folding the negation at
generation time, which I don't think the current version would.
(Sorry if that's wrong.)

Richard

Re: [Patch] 1/2 Expand sync_sub as sync_add if predicates don't match.

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 2891 bytes --]

Richard Sandiford wrote:
> David Daney <ddaney@avtrex.com> writes:
>   
>> While implementing the atomic memory operations for MIPS, I came across 
>> a problem where immediate operands were forced into a register when 
>> sync_sub* insns were expanded.  On MIPS there is no minus instruction 
>> for immediate operands, instead this should be expanded as plus with the 
>> negative value of the immediate operand.
>>
>> In expand_sync_operation() and expand_sync_fetch_operation() there is 
>> code to handle conversion to plus if the minus insn does not exits.  I 
>> enhanced this slightly so that it also does the conversion if the 
>> predicate for sync_sub* does not match, but it does match for the 
>> corresponding sync_add*.  This allows me to write sync_sub* insns with a 
>> register predicate and have them used for non-immediate operands, but an 
>> immediate operand results in conversion to sync_add*.
>>
>> Tested on x86_64-pc-linux-gnu all default languages both native and -m32 
>> with no regressions.  Also tested mips64-linux C only.
>>
>> :ADDPATCH middle-end:
>>
>> OK to commit?
>>     
>
> FWIW, with (minus ... (const_int ...)) not being canonical rtl, I think
> we should always use PLUS rather than MINUS if the operand is a CONST_INT.
> E.g. the first thing expand_binop does is:
>
>   /* If subtracting an integer constant, convert this into an addition of
>      the negated constant.  */
>   if (binoptab == sub_optab && GET_CODE (op1) == CONST_INT)
>     {
>       op1 = negate_rtx (mode, op1);
>       binoptab = add_optab;
>     }
>
> and I think we should have a similar thing in expand_sync_operation
> and expand_sync_fetch_operation.  As well as avoiding invalid
> expressions, it has the advantage of folding the negation at
> generation time, which I don't think the current version would.
> (Sorry if that's wrong.)
>   

The attached new revision of the patch does it this way now.  Light 
testing on MIPS shows that it has the same effect as the first version.  
Which from the point of view of my mips atomic memory operations patch 
is sufficient.

I was a little hesitant to do it this way the first time because it has 
the potential of adversely impacting a (hypothetical) port where

sync_sub<mode> is more efficient than sync_add<mode> the only place I could think of where this would be plausible is for cases where negating the value would cause it to no longer fit in the same number of bits as the original value (-0x8000 on mips assuming there were a subiu instruction).

If deemed better than the previous version, I will fully test this.  It 
would however be nice to get feedback that this is really better before 
exerting a lot more effort in testing.

2007-08-21  David Daney  <ddaney@avtrex.com>

    * optabs.c (expand_sync_operation):  Use plus insn if minus
    CONST_INT_P(val).
    (expand_sync_fetch_operation):  Ditto.


[-- Attachment #2: optabs.1.diff --]
[-- Type: text/x-patch, Size: 866 bytes --]

Index: optabs.c
===================================================================
--- optabs.c	(revision 127356)
+++ optabs.c	(working copy)
@@ -6444,7 +6444,7 @@ expand_sync_operation (rtx mem, rtx val,
 
     case MINUS:
       icode = sync_sub_optab[mode];
-      if (icode == CODE_FOR_nothing)
+      if (icode == CODE_FOR_nothing || CONST_INT_P (val))
 	{
 	  icode = sync_add_optab[mode];
 	  if (icode != CODE_FOR_nothing)
@@ -6544,7 +6544,8 @@ expand_sync_fetch_operation (rtx mem, rt
     case MINUS:
       old_code = sync_old_sub_optab[mode];
       new_code = sync_new_sub_optab[mode];
-      if (old_code == CODE_FOR_nothing && new_code == CODE_FOR_nothing)
+      if ((old_code == CODE_FOR_nothing && new_code == CODE_FOR_nothing)
+          || CONST_INT_P (val))
 	{
 	  old_code = sync_old_add_optab[mode];
 	  new_code = sync_new_add_optab[mode];

Re: [Patch] 1/2 Expand sync_sub as sync_add if predicates don't match.

[David Daney]

David Daney wrote:
> Richard Sandiford wrote:
>> David Daney <ddaney@avtrex.com> writes:
>>  
>>> While implementing the atomic memory operations for MIPS, I came 
>>> across a problem where immediate operands were forced into a register 
>>> when sync_sub* insns were expanded.  On MIPS there is no minus 
>>> instruction for immediate operands, instead this should be expanded 
>>> as plus with the negative value of the immediate operand.
>>>
>>> In expand_sync_operation() and expand_sync_fetch_operation() there is 
>>> code to handle conversion to plus if the minus insn does not exits.  
>>> I enhanced this slightly so that it also does the conversion if the 
>>> predicate for sync_sub* does not match, but it does match for the 
>>> corresponding sync_add*.  This allows me to write sync_sub* insns 
>>> with a register predicate and have them used for non-immediate 
>>> operands, but an immediate operand results in conversion to sync_add*.
>>>
>>> Tested on x86_64-pc-linux-gnu all default languages both native and 
>>> -m32 with no regressions.  Also tested mips64-linux C only.
>>>
>>> :ADDPATCH middle-end:
>>>
>>> OK to commit?
>>>     
>>
>> FWIW, with (minus ... (const_int ...)) not being canonical rtl, I think
>> we should always use PLUS rather than MINUS if the operand is a 
>> CONST_INT.
>> E.g. the first thing expand_binop does is:
>>
>>   /* If subtracting an integer constant, convert this into an addition of
>>      the negated constant.  */
>>   if (binoptab == sub_optab && GET_CODE (op1) == CONST_INT)
>>     {
>>       op1 = negate_rtx (mode, op1);
>>       binoptab = add_optab;
>>     }
>>
>> and I think we should have a similar thing in expand_sync_operation
>> and expand_sync_fetch_operation.  As well as avoiding invalid
>> expressions, it has the advantage of folding the negation at
>> generation time, which I don't think the current version would.
>> (Sorry if that's wrong.)
>>   
> 
> The attached new revision of the patch does it this way now.  Light 
> testing on MIPS shows that it has the same effect as the first version.  
> Which from the point of view of my mips atomic memory operations patch 
> is sufficient.
> 
> I was a little hesitant to do it this way the first time because it has 
> the potential of adversely impacting a (hypothetical) port where
> 
> sync_sub<mode> is more efficient than sync_add<mode> the only place I 
> could think of where this would be plausible is for cases where negating 
> the value would cause it to no longer fit in the same number of bits as 
> the original value (-0x8000 on mips assuming there were a subiu 
> instruction).
> 
> If deemed better than the previous version, I will fully test this.  It 
> would however be nice to get feedback that this is really better before 
> exerting a lot more effort in testing.

Now tested on both i686-pc-linux-gnu and x86_64-pc-linux-gnu all default 
languages with no regressions found.

OK to commit?


> 
> 2007-08-21  David Daney  <ddaney@avtrex.com>
> 
>    * optabs.c (expand_sync_operation):  Use plus insn if minus
>    CONST_INT_P(val).
>    (expand_sync_fetch_operation):  Ditto.
> 
> 
> ------------------------------------------------------------------------
> 
> Index: optabs.c
> ===================================================================
> --- optabs.c	(revision 127356)
> +++ optabs.c	(working copy)
> @@ -6444,7 +6444,7 @@ expand_sync_operation (rtx mem, rtx val,
>  
>      case MINUS:
>        icode = sync_sub_optab[mode];
> -      if (icode == CODE_FOR_nothing)
> +      if (icode == CODE_FOR_nothing || CONST_INT_P (val))
>  	{
>  	  icode = sync_add_optab[mode];
>  	  if (icode != CODE_FOR_nothing)
> @@ -6544,7 +6544,8 @@ expand_sync_fetch_operation (rtx mem, rt
>      case MINUS:
>        old_code = sync_old_sub_optab[mode];
>        new_code = sync_new_sub_optab[mode];
> -      if (old_code == CODE_FOR_nothing && new_code == CODE_FOR_nothing)
> +      if ((old_code == CODE_FOR_nothing && new_code == CODE_FOR_nothing)
> +          || CONST_INT_P (val))
>  	{
>  	  old_code = sync_old_add_optab[mode];
>  	  new_code = sync_new_add_optab[mode];

Ping: [Patch] 1/2 Expand sync_sub as sync_add if predicates don't match.

[David Daney]

This patch is waiting for a middle-end maintainer review.

Also note the follow-up comments in:

http://gcc.gnu.org/ml/gcc-patches/2007-09/msg00090.html

Thanks,
David Daney

David Daney wrote:
> David Daney wrote:
>> Richard Sandiford wrote:
>>> David Daney <ddaney@avtrex.com> writes:
>>>  
>>>> While implementing the atomic memory operations for MIPS, I came 
>>>> across a problem where immediate operands were forced into a 
>>>> register when sync_sub* insns were expanded.  On MIPS there is no 
>>>> minus instruction for immediate operands, instead this should be 
>>>> expanded as plus with the negative value of the immediate operand.
>>>>
>>>> In expand_sync_operation() and expand_sync_fetch_operation() there 
>>>> is code to handle conversion to plus if the minus insn does not 
>>>> exits.  I enhanced this slightly so that it also does the conversion 
>>>> if the predicate for sync_sub* does not match, but it does match for 
>>>> the corresponding sync_add*.  This allows me to write sync_sub* 
>>>> insns with a register predicate and have them used for non-immediate 
>>>> operands, but an immediate operand results in conversion to sync_add*.
>>>>
>>>> Tested on x86_64-pc-linux-gnu all default languages both native and 
>>>> -m32 with no regressions.  Also tested mips64-linux C only.
>>>>
>>>> :ADDPATCH middle-end:
>>>>
>>>> OK to commit?
>>>>     
>>>
>>> FWIW, with (minus ... (const_int ...)) not being canonical rtl, I think
>>> we should always use PLUS rather than MINUS if the operand is a 
>>> CONST_INT.
>>> E.g. the first thing expand_binop does is:
>>>
>>>   /* If subtracting an integer constant, convert this into an 
>>> addition of
>>>      the negated constant.  */
>>>   if (binoptab == sub_optab && GET_CODE (op1) == CONST_INT)
>>>     {
>>>       op1 = negate_rtx (mode, op1);
>>>       binoptab = add_optab;
>>>     }
>>>
>>> and I think we should have a similar thing in expand_sync_operation
>>> and expand_sync_fetch_operation.  As well as avoiding invalid
>>> expressions, it has the advantage of folding the negation at
>>> generation time, which I don't think the current version would.
>>> (Sorry if that's wrong.)
>>>   
>>
>> The attached new revision of the patch does it this way now.  Light 
>> testing on MIPS shows that it has the same effect as the first 
>> version.  Which from the point of view of my mips atomic memory 
>> operations patch is sufficient.
>>
>> I was a little hesitant to do it this way the first time because it 
>> has the potential of adversely impacting a (hypothetical) port where
>>
>> sync_sub<mode> is more efficient than sync_add<mode> the only place I 
>> could think of where this would be plausible is for cases where 
>> negating the value would cause it to no longer fit in the same number 
>> of bits as the original value (-0x8000 on mips assuming there were a 
>> subiu instruction).
>>
>> If deemed better than the previous version, I will fully test this.  
>> It would however be nice to get feedback that this is really better 
>> before exerting a lot more effort in testing.
> 
> Now tested on both i686-pc-linux-gnu and x86_64-pc-linux-gnu all default 
> languages with no regressions found.
> 
> OK to commit?
> 
> 
>>
>> 2007-08-21  David Daney  <ddaney@avtrex.com>
>>
>>    * optabs.c (expand_sync_operation):  Use plus insn if minus
>>    CONST_INT_P(val).
>>    (expand_sync_fetch_operation):  Ditto.
>>
>>
>> ------------------------------------------------------------------------
>>
>> Index: optabs.c
>> ===================================================================
>> --- optabs.c    (revision 127356)
>> +++ optabs.c    (working copy)
>> @@ -6444,7 +6444,7 @@ expand_sync_operation (rtx mem, rtx val,
>>  
>>      case MINUS:
>>        icode = sync_sub_optab[mode];
>> -      if (icode == CODE_FOR_nothing)
>> +      if (icode == CODE_FOR_nothing || CONST_INT_P (val))
>>      {
>>        icode = sync_add_optab[mode];
>>        if (icode != CODE_FOR_nothing)
>> @@ -6544,7 +6544,8 @@ expand_sync_fetch_operation (rtx mem, rt
>>      case MINUS:
>>        old_code = sync_old_sub_optab[mode];
>>        new_code = sync_new_sub_optab[mode];
>> -      if (old_code == CODE_FOR_nothing && new_code == CODE_FOR_nothing)
>> +      if ((old_code == CODE_FOR_nothing && new_code == CODE_FOR_nothing)
>> +          || CONST_INT_P (val))
>>      {
>>        old_code = sync_old_add_optab[mode];
>>        new_code = sync_new_add_optab[mode];
> 

Ping^3: use sync_add rather than sync_sub for CONST_INTs

[Richard Sandiford]

I'd like to ping David's patch (third time of asking):

    http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01417.html

It simply makes expand canonicalise sync_sub of a constant as sync_add,
just like expand does for plain "sub" and "add".  As it stands,
the target's sync_sub either has to accept a CONST_INT -- which for
patterns like alpha's and MIPS's would lead to noncanonical rtl --
or it has to unnecessarily force the constant into a register.

Richard

David Daney <ddaney@avtrex.com> writes:
> This patch is waiting for a middle-end maintainer review.
>
> Also note the follow-up comments in:
>
> http://gcc.gnu.org/ml/gcc-patches/2007-09/msg00090.html
>
> Thanks,
> David Daney
>
> David Daney wrote:
>> David Daney wrote:
>>> Richard Sandiford wrote:
>>>> David Daney <ddaney@avtrex.com> writes:
>>>>  
>>>>> While implementing the atomic memory operations for MIPS, I came 
>>>>> across a problem where immediate operands were forced into a 
>>>>> register when sync_sub* insns were expanded.  On MIPS there is no 
>>>>> minus instruction for immediate operands, instead this should be 
>>>>> expanded as plus with the negative value of the immediate operand.
>>>>>
>>>>> In expand_sync_operation() and expand_sync_fetch_operation() there 
>>>>> is code to handle conversion to plus if the minus insn does not 
>>>>> exits.  I enhanced this slightly so that it also does the conversion 
>>>>> if the predicate for sync_sub* does not match, but it does match for 
>>>>> the corresponding sync_add*.  This allows me to write sync_sub* 
>>>>> insns with a register predicate and have them used for non-immediate 
>>>>> operands, but an immediate operand results in conversion to sync_add*.
>>>>>
>>>>> Tested on x86_64-pc-linux-gnu all default languages both native and 
>>>>> -m32 with no regressions.  Also tested mips64-linux C only.
>>>>>
>>>>> :ADDPATCH middle-end:
>>>>>
>>>>> OK to commit?
>>>>>     
>>>>
>>>> FWIW, with (minus ... (const_int ...)) not being canonical rtl, I think
>>>> we should always use PLUS rather than MINUS if the operand is a 
>>>> CONST_INT.
>>>> E.g. the first thing expand_binop does is:
>>>>
>>>>   /* If subtracting an integer constant, convert this into an 
>>>> addition of
>>>>      the negated constant.  */
>>>>   if (binoptab == sub_optab && GET_CODE (op1) == CONST_INT)
>>>>     {
>>>>       op1 = negate_rtx (mode, op1);
>>>>       binoptab = add_optab;
>>>>     }
>>>>
>>>> and I think we should have a similar thing in expand_sync_operation
>>>> and expand_sync_fetch_operation.  As well as avoiding invalid
>>>> expressions, it has the advantage of folding the negation at
>>>> generation time, which I don't think the current version would.
>>>> (Sorry if that's wrong.)
>>>>   
>>>
>>> The attached new revision of the patch does it this way now.  Light 
>>> testing on MIPS shows that it has the same effect as the first 
>>> version.  Which from the point of view of my mips atomic memory 
>>> operations patch is sufficient.
>>>
>>> I was a little hesitant to do it this way the first time because it 
>>> has the potential of adversely impacting a (hypothetical) port where
>>>
>>> sync_sub<mode> is more efficient than sync_add<mode> the only place I 
>>> could think of where this would be plausible is for cases where 
>>> negating the value would cause it to no longer fit in the same number 
>>> of bits as the original value (-0x8000 on mips assuming there were a 
>>> subiu instruction).
>>>
>>> If deemed better than the previous version, I will fully test this.  
>>> It would however be nice to get feedback that this is really better 
>>> before exerting a lot more effort in testing.
>> 
>> Now tested on both i686-pc-linux-gnu and x86_64-pc-linux-gnu all default 
>> languages with no regressions found.
>> 
>> OK to commit?
>> 
>> 
>>>
>>> 2007-08-21  David Daney  <ddaney@avtrex.com>
>>>
>>>    * optabs.c (expand_sync_operation):  Use plus insn if minus
>>>    CONST_INT_P(val).
>>>    (expand_sync_fetch_operation):  Ditto.
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>> Index: optabs.c
>>> ===================================================================
>>> --- optabs.c    (revision 127356)
>>> +++ optabs.c    (working copy)
>>> @@ -6444,7 +6444,7 @@ expand_sync_operation (rtx mem, rtx val,
>>>  
>>>      case MINUS:
>>>        icode = sync_sub_optab[mode];
>>> -      if (icode == CODE_FOR_nothing)
>>> +      if (icode == CODE_FOR_nothing || CONST_INT_P (val))
>>>      {
>>>        icode = sync_add_optab[mode];
>>>        if (icode != CODE_FOR_nothing)
>>> @@ -6544,7 +6544,8 @@ expand_sync_fetch_operation (rtx mem, rt
>>>      case MINUS:
>>>        old_code = sync_old_sub_optab[mode];
>>>        new_code = sync_new_sub_optab[mode];
>>> -      if (old_code == CODE_FOR_nothing && new_code == CODE_FOR_nothing)
>>> +      if ((old_code == CODE_FOR_nothing && new_code == CODE_FOR_nothing)
>>> +          || CONST_INT_P (val))
>>>      {
>>>        old_code = sync_old_add_optab[mode];
>>>        new_code = sync_new_add_optab[mode];
>> 

Re: Ping^3: use sync_add rather than sync_sub for CONST_INTs

[Richard Guenther]

On 9/9/07, Richard Sandiford <richard@codesourcery.com> wrote:
> I'd like to ping David's patch (third time of asking):
>
>     http://gcc.gnu.org/ml/gcc-patches/2007-08/msg01417.html
>
> It simply makes expand canonicalise sync_sub of a constant as sync_add,
> just like expand does for plain "sub" and "add".  As it stands,
> the target's sync_sub either has to accept a CONST_INT -- which for
> patterns like alpha's and MIPS's would lead to noncanonical rtl --
> or it has to unnecessarily force the constant into a register.

Makes sense and thus ok.  I suppose there are no targets that have
optabs for sync_sub but not sync_add? :)

Thanks,
Richard.