Re: [Patch] MIPS: Add atomic memory operations for QI and HI modes.

[Richard Sandiford <rsandifo at nildram dot co dot uk>]

Hi David,

David Daney <ddaney@avtrex.com> writes:
> David Daney wrote:
>> This patch adds support for QI and HI mode atomic memory operations to
>> MIPS.  This is a minimal implementation of the QI and HI mode operations
>> as it only implements sync_compare_and_swap for those modes.  A possible
>> future enhancement would be to implement the full set of atomic insns
>> for QI and HI modes, but this should be enough to get libstdc++ building
>> again.
>> 
>> The patch borrows from the sparc implementation.
>> 
>> Currently testing on mipsel-linux with all languages,
>> 
>> OK to commit if it tests good?
>
> And it did:
>
> http://gcc.gnu.org/ml/gcc-testresults/2008-04/msg01887.html
>
> This gets us back to clean C++ and libstdc++ test results (except for 
> pr31863.C, which times out occasionally when the system is heavily loaded).

First of all, thanks a lot for doing this.  The patch looks good.

As far as correctness goes, I think the SImode in:

  mips_emit_binary (AND, addr, addr1, force_reg (SImode, GEN_INT (-4)));

ought to be Pmode.  There's also a missing conversion from Pmode
to SImode when computing the shift.

We also need to adjust gcc.target/mips/gcc-have-compare-and-swap-[12].c,
which doesn't expect the byte and halfword versions to exist.

Sorry once again for the annoying nitpicky micromanagement, but I'm a
little uncomfortable having insn-generating things like force_reg in an
initialiser, especially when mixed with simpler things like gen_reg_rtx.
I realise that you're just copying that part from sparc.

Also:

  if (GET_MODE (mem) == QImode)
    unshifted_mask = force_reg (SImode, GEN_INT (0xff));
  else
    unshifted_mask = force_reg (SImode, GEN_INT (0xffff));

can be simplied using GET_MODE_MASK (GET_MODE (mem)).

Finally, I thought it would be worth having a version of mips_emit_binary
that creates a new register.  That should make it easier to see where a
value is first initialised.

Here's a version of the patch with those nits picked.  I also added
a few more comments.  (I'm sure some folk will think it's now very
much over-commented...)

I won't have time to test this for a few days, as my machine is busy
with other things.  But I tested that it produces the same code as
your patch for the gcc.torture/compile/sync-1.c test for o32 and n32,
and it makes that test work for n64.

What do you think?  Please let me know if you don't like any
of the changes.

Richard


gcc/
2008-04-23  David Daney  <ddaney@avtrex.com>

	* config/mips/mips.md (UNSPEC_COMPARE_AND_SWAP_12): New
	unspec_volitile.
	(UNSPEC_SYNC_OLD_OP, UNSPEC_SYNC_NEW_OP, UNSPEC_SYNC_EXCHANGE,
	UNSPEC_MEMORY_BARRIER, UNSPEC_SET_GOT_VERSION,
	UNSPEC_UPDATE_GOT_VERSION): Renumber.
	(sync_compare_and_swap<mode>): New expand for QI and HI modes.
	(compare_and_swap_12): New insn.
	* config/mips/mips-protos.h (mips_expand_compare_and_swap_12): Declare.
	* config/mips/mips.c (mips_force_binary): New function.
	(mips_emit_int_order_test, mips_expand_synci_loop): Use it.
	(mips_expand_compare_and_swap_12): New function.
	* config/mips/mips.h (MIPS_COMPARE_AND_SWAP_12): New macro.

gcc/testsuite/
	* gcc.target/mips/gcc-have-sync-compare-and-swap-1.c: Expect
	__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 and
	__GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 to be defined.
	* gcc.target/mips/gcc-have-sync-compare-and-swap-2.c: Likewise.

Index: gcc/config/mips/mips.md
===================================================================
--- gcc/config/mips/mips.md	2008-04-24 18:43:00.000000000 +0100
+++ gcc/config/mips/mips.md	2008-04-24 18:43:11.000000000 +0100
@@ -54,12 +54,13 @@ (define_constants
    (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_MEMORY_BARRIER	41)
-   (UNSPEC_SET_GOT_VERSION	42)
-   (UNSPEC_UPDATE_GOT_VERSION	43)
+   (UNSPEC_COMPARE_AND_SWAP_12	38)
+   (UNSPEC_SYNC_OLD_OP		39)
+   (UNSPEC_SYNC_NEW_OP		40)
+   (UNSPEC_SYNC_EXCHANGE	41)
+   (UNSPEC_MEMORY_BARRIER	42)
+   (UNSPEC_SET_GOT_VERSION	43)
+   (UNSPEC_UPDATE_GOT_VERSION	44)
    
    (UNSPEC_ADDRESS_FIRST	100)
 
@@ -4447,6 +4448,34 @@ (define_insn "sync_compare_and_swap<mode
 }
   [(set_attr "length" "32")])
 
+(define_expand "sync_compare_and_swap<mode>"
+  [(match_operand:SHORT 0 "register_operand")
+   (match_operand:SHORT 1 "memory_operand")
+   (match_operand:SHORT 2 "general_operand")
+   (match_operand:SHORT 3 "general_operand")]
+  "GENERATE_LL_SC"
+{
+  mips_expand_compare_and_swap_12 (operands[0], operands[1],
+				   operands[2], operands[3]);
+  DONE;
+})
+
+;; Helper insn for mips_expand_compare_and_swap_12.
+(define_insn "compare_and_swap_12"
+  [(set (match_operand:SI 0 "register_operand" "=&d")
+	(match_operand:SI 1 "memory_operand" "+R"))
+   (set (match_dup 1)
+	(unspec_volatile:SI [(match_operand:SI 2 "register_operand" "d")
+			     (match_operand:SI 3 "register_operand" "d")
+			     (match_operand:SI 4 "register_operand" "d")
+			     (match_operand:SI 5 "register_operand" "d")]
+			    UNSPEC_COMPARE_AND_SWAP_12))]
+  "GENERATE_LL_SC"
+{
+  return MIPS_COMPARE_AND_SWAP_12;
+}
+  [(set_attr "length" "40")])
+
 (define_insn "sync_add<mode>"
   [(set (match_operand:GPR 0 "memory_operand" "+R,R")
 	(unspec_volatile:GPR
Index: gcc/config/mips/mips-protos.h
===================================================================
--- gcc/config/mips/mips-protos.h	2008-04-24 18:43:00.000000000 +0100
+++ gcc/config/mips/mips-protos.h	2008-04-24 18:43:11.000000000 +0100
@@ -292,5 +292,6 @@ extern bool mips_use_ins_ext_p (rtx, HOS
 extern const char *mips16e_output_save_restore (rtx, HOST_WIDE_INT);
 extern bool mips16e_save_restore_pattern_p (rtx, HOST_WIDE_INT,
 					    struct mips16e_save_restore_info *);
+extern void mips_expand_compare_and_swap_12 (rtx, rtx, rtx, rtx);
 
 #endif /* ! GCC_MIPS_PROTOS_H */
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	2008-04-24 18:43:00.000000000 +0100
+++ gcc/config/mips/mips.c	2008-04-24 19:40:15.000000000 +0100
@@ -1,6 +1,6 @@
 /* Subroutines used for MIPS code generation.
    Copyright (C) 1989, 1990, 1991, 1993, 1994, 1995, 1996, 1997, 1998,
-   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007
+   1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008
    Free Software Foundation, Inc.
    Contributed by A. Lichnewsky, lich@inria.inria.fr.
    Changes by Michael Meissner, meissner@osf.org.
@@ -2121,6 +2121,19 @@ mips_emit_binary (enum rtx_code code, rt
 			  gen_rtx_fmt_ee (code, GET_MODE (target), op0, op1)));
 }
 
+/* Compute (CODE OP0 OP1) and store the result in a new register
+   of mode MODE.  Return that new register.  */
+
+static rtx
+mips_force_binary (enum machine_mode mode, enum rtx_code code, rtx op0, rtx op1)
+{
+  rtx reg;
+
+  reg = gen_reg_rtx (mode);
+  mips_emit_binary (code, reg, op0, op1);
+  return reg;
+}
+
 /* Copy VALUE to a register and return that register.  If new pseudos
    are allowed, copy it into a new register, otherwise use DEST.  */
 
@@ -3741,8 +3754,10 @@ mips_emit_int_order_test (enum rtx_code 
 	}
       else if (invert_ptr == 0)
 	{
-	  rtx inv_target = gen_reg_rtx (GET_MODE (target));
-	  mips_emit_binary (inv_code, inv_target, cmp0, cmp1);
+	  rtx inv_target;
+
+	  inv_target = mips_force_binary (GET_MODE (target),
+					  inv_code, cmp0, cmp1);
 	  mips_emit_binary (XOR, target, inv_target, const1_rtx);
 	}
       else
@@ -5850,8 +5865,7 @@ mips_expand_synci_loop (rtx begin, rtx e
 
   emit_insn (gen_synci (begin));
 
-  cmp = gen_reg_rtx (Pmode);
-  mips_emit_binary (GTU, cmp, begin, end);
+  cmp = mips_force_binary (Pmode, GTU, begin, end);
 
   mips_emit_binary (PLUS, begin, begin, inc);
 
@@ -5859,6 +5873,68 @@ mips_expand_synci_loop (rtx begin, rtx e
   emit_jump_insn (gen_condjump (cmp_result, label));
 }
 
+/* Expand a QI or HI mode compare_and_swap.  The operands are the same
+   as for the generator function.  */
+
+void
+mips_expand_compare_and_swap_12 (rtx result, rtx mem, rtx oldval, rtx newval)
+{
+  rtx orig_addr, memsi_addr, memsi, shift, shiftsi, unshifted_mask;
+  rtx mask, inverted_mask, oldvalsi, old_shifted, newvalsi, new_shifted, res;
+
+  /* Compute the address of the containing SImode value.  */
+  orig_addr = force_reg (Pmode, XEXP (mem, 0));
+  memsi_addr = mips_force_binary (Pmode, AND, orig_addr,
+				  force_reg (Pmode, GEN_INT (-4)));
+
+  /* Create a memory reference for it.  */
+  memsi = gen_rtx_MEM (SImode, memsi_addr);
+  set_mem_alias_set (memsi, ALIAS_SET_MEMORY_BARRIER);
+  MEM_VOLATILE_P (memsi) = MEM_VOLATILE_P (mem);
+
+  /* Work out the byte offset of the QImode or HImode value,
+     counting from the least significant byte.  */
+  shift = mips_force_binary (Pmode, AND, orig_addr, GEN_INT (3));
+  if (TARGET_BIG_ENDIAN)
+    mips_emit_binary (XOR, shift, shift,
+		      GEN_INT (GET_MODE (mem) == QImode ? 3 : 2));
+
+  /* Multiply by eight to convert the shift value from bytes to bits.  */
+  mips_emit_binary (ASHIFT, shift, shift, GEN_INT (3));
+
+  /* Make the final shift an SImode value, so that it can be used in
+     SImode operations.  */
+  shiftsi = force_reg (SImode, gen_lowpart (SImode, shift));
+
+  /* Set MASK to an inclusive mask of the QImode or HImode value.  */
+  unshifted_mask = GEN_INT (GET_MODE_MASK (GET_MODE (mem)));
+  unshifted_mask = force_reg (SImode, unshifted_mask);
+  mask = mips_force_binary (SImode, ASHIFT, unshifted_mask, shiftsi);
+
+  /* Compute the equivalent exclusive mask.  */
+  inverted_mask = gen_reg_rtx (SImode);
+  emit_insn (gen_rtx_SET (VOIDmode, inverted_mask,
+			  gen_rtx_NOT (SImode, mask)));
+
+  /* Shift the old value into place.  */
+  oldvalsi = force_reg (SImode, gen_lowpart (SImode, oldval));
+  old_shifted = mips_force_binary (SImode, ASHIFT, oldvalsi, shiftsi);
+
+  /* Do the same for the new value.  */
+  newvalsi = force_reg (SImode, gen_lowpart (SImode, newval));
+  new_shifted = mips_force_binary (SImode, ASHIFT, newvalsi, shiftsi);
+
+  /* Do the SImode atomic access.  */
+  res = gen_reg_rtx (SImode);
+  emit_insn (gen_compare_and_swap_12 (res, memsi, mask, inverted_mask,
+				      old_shifted, new_shifted));
+
+  /* Shift and convert the result.  */
+  mips_emit_binary (AND, res, res, mask);
+  mips_emit_binary (LSHIFTRT, res, res, shiftsi);
+  mips_emit_move (result, gen_lowpart (GET_MODE (result), res));
+}
+
 /* Return true if it is possible to use left/right accesses for a
    bitfield of WIDTH bits starting BITPOS bits into *OP.  When
    returning true, update *OP, *LEFT and *RIGHT as follows:
Index: gcc/config/mips/mips.h
===================================================================
--- gcc/config/mips/mips.h	2008-04-24 18:43:00.000000000 +0100
+++ gcc/config/mips/mips.h	2008-04-24 18:43:11.000000000 +0100
@@ -2906,6 +2906,30 @@ #define MIPS_COMPARE_AND_SWAP(SUFFIX, OP
 
 /* Return an asm string that atomically:
 
+     - Given that %2 contains a bit mask and %3 the inverted mask and
+       that %4 and %5 have already been ANDed with $2.
+
+     - Compares the bits in memory reference %1 selected by mask %2 to
+       register %4 and, if they are equal, changes the selected bits
+       in memory to %5.
+
+     - Sets register %0 to the old value of memory reference %1.
+ */
+#define MIPS_COMPARE_AND_SWAP_12		\
+  "%(%<%[%|sync\n"				\
+  "1:\tll\t%0,%1\n"				\
+  "\tand\t%@,%0,%2\n"				\
+  "\tbne\t%@,%4,2f\n"				\
+  "\tand\t%@,%0,%3\n"				\
+  "\tor\t%@,%@,%5\n"				\
+  "\tsc\t%@,%1\n"				\
+  "\tbeq\t%@,%.,1b\n"				\
+  "\tnop\n"					\
+  "\tsync%-%]%>%)\n"				\
+  "2:\n"
+
+/* 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"
Index: gcc/testsuite/gcc.target/mips/gcc-have-sync-compare-and-swap-1.c
===================================================================
--- gcc/testsuite/gcc.target/mips/gcc-have-sync-compare-and-swap-1.c	2008-04-24 19:01:33.000000000 +0100
+++ gcc/testsuite/gcc.target/mips/gcc-have-sync-compare-and-swap-1.c	2008-04-24 19:01:58.000000000 +0100
@@ -1,11 +1,11 @@
 /* { dg-do preprocess } */
 /* { dg-mips-options "-mips2" } */
 
-#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1
+#if defined (__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1) == defined (__mips16)
 #error nonono
 #endif
 
-#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2
+#if defined (__GCC_HAVE_SYNC_COMPARE_AND_SWAP_2) == defined (__mips16)
 #error nonono
 #endif
 
Index: gcc/testsuite/gcc.target/mips/gcc-have-sync-compare-and-swap-2.c
===================================================================
--- gcc/testsuite/gcc.target/mips/gcc-have-sync-compare-and-swap-2.c	2008-04-24 19:02:15.000000000 +0100
+++ gcc/testsuite/gcc.target/mips/gcc-have-sync-compare-and-swap-2.c	2008-04-24 19:02:25.000000000 +0100
@@ -1,11 +1,11 @@
 /* { dg-do preprocess } */
 /* { dg-mips-options "-mgp64" } */
 
-#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1
+#if defined (__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1) == defined (__mips16)
 #error nonono
 #endif
 
-#ifdef __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2
+#if defined (__GCC_HAVE_SYNC_COMPARE_AND_SWAP_2) == defined (__mips16)
 #error nonono
 #endif