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: [RX] Add support for atomic operations


On Mon, 2016-05-09 at 15:18 +0100, Nick Clifton wrote:

> > gcc/ChangeLog:
> > 	* config/rx/rx-protos.h (is_interrupt_func,
> > is_fast_interrupt_func):
> > 	Forward declare.
> > 	(rx_atomic_sequence): New class.
> > 	* config/rx/rx.c (rx_print_operand): Use symbolic names for PSW
> > bits.
> > 	(is_interrupt_func, is_fast_interrupt_func): Make non-static
> > and
> > 	non-inline.
> > 	(rx_atomic_sequence::rx_atomic_sequence,
> > 	rx_atomic_sequence::~rx_atomic_sequence): New functions.
> > 	* config/rx/rx.md (CTRLREG_PSW, CTRLREG_USP, CTRLREG_FPSW,
> > CTRLREG_CPEN,
> > 	CTRLREG_BPSW, CTRLREG_BPC, CTRLREG_ISP, CTRLREG_FINTV,
> > 	CTRLREG_INTB): New constants.
> > 	(FETCHOP): New code iterator.
> > 	(fethcop_name, fetchop_name2): New iterator code attributes.
> > 	(QIHI): New mode iterator.
> > 	(atomic_exchange<mode>, atomic_exchangesi, xchg_mem<mode>,
> > 	atomic_fetch_<fetchop_name>si, atomic_fetch_nandsi,
> > 	atomic_<fetchop_name>_fetchsi, atomic_nand_fetchsi): New
> > patterns.
> 
> Approved - please apply.
> 

Sorry, but my original patch was buggy.  There are two problems:

First, when interrupts are re-enabled by restoring the PSW using the
"mvtc" insn after the atomic sequence, the CC_REG is clobbered.  It's
not entirely clear to me why leaving out the CC_REG clobber in "mvtc"
is of any benefit.  Instead of adding a new "mvtc" pattern, I've just
added the clobber to the existing one.  With that wrong code issues
around atomic sequences such as atomic decrement and test for zero are
fixed.

Second, the atomic_<fetchop_name>_fetchsi works only with commutative
operations because the memory operand and the register operand are
swapped in the expander.  Thus it produces wrong code for subtraction
operations.  The fix is to use a separate pattern for subtraction and
not twist the operands.

The attached patch fixes those issues.
OK for trunk?

Cheers,
Oleg

gcc/ChangeLog:
	* config/rx/rx.md (FETCHOP_NO_MINUS): New code iterator.
	(atomic_<fetchop_name>_fetchsi): Extract minus operator into ...
	(atomic_sub_fetchsi): ... this new pattern.
	(mvtc): Add CC_REG clobber.
Index: gcc/config/rx/rx.md
===================================================================
--- gcc/config/rx/rx.md	(revision 236761)
+++ gcc/config/rx/rx.md	(working copy)
@@ -2158,6 +2158,7 @@
 ;; Atomic operations.
 
 (define_code_iterator FETCHOP [plus minus ior xor and])
+(define_code_iterator FETCHOP_NO_MINUS [plus ior xor and])
 
 (define_code_attr fetchop_name
   [(plus "add") (minus "sub") (ior "or") (xor "xor") (and "and")])
@@ -2258,12 +2259,14 @@
 })
 
 ;; read - modify - write - return new value
+;; Because subtraction is not commutative we need to specify a different
+;; set of patterns for it.
 (define_expand "atomic_<fetchop_name>_fetchsi"
   [(set (match_operand:SI 0 "register_operand")
-	(FETCHOP:SI (match_operand:SI 1 "rx_restricted_mem_operand")
-		    (match_operand:SI 2 "register_operand")))
+	(FETCHOP_NO_MINUS:SI (match_operand:SI 1 "rx_restricted_mem_operand")
+			     (match_operand:SI 2 "register_operand")))
    (set (match_dup 1)
-	(FETCHOP:SI (match_dup 1) (match_dup 2)))
+	(FETCHOP_NO_MINUS:SI (match_dup 1) (match_dup 2)))
    (match_operand:SI 3 "const_int_operand")]		;; memory model
   ""
 {
@@ -2277,6 +2280,25 @@
   DONE;
 })
 
+(define_expand "atomic_sub_fetchsi"
+  [(set (match_operand:SI 0 "register_operand")
+	(minus:SI (match_operand:SI 1 "rx_restricted_mem_operand")
+		  (match_operand:SI 2 "rx_source_operand")))
+   (set (match_dup 1)
+	(minus:SI (match_dup 1) (match_dup 2)))
+   (match_operand:SI 3 "const_int_operand")]		;; memory model
+  ""
+{
+  {
+    rx_atomic_sequence seq (current_function_decl);
+
+    emit_move_insn (operands[0], operands[1]);
+    emit_insn (gen_subsi3 (operands[0], operands[0], operands[2]));
+    emit_move_insn (operands[1], operands[0]);
+  }
+  DONE;
+})
+
 (define_expand "atomic_nand_fetchsi"
   [(set (match_operand:SI 0 "register_operand")
 	(not:SI (and:SI (match_operand:SI 1 "rx_restricted_mem_operand")
@@ -2674,18 +2696,16 @@
 )
 
 ;; Move to control register
+;; This insn can be used in atomic sequences to restore the previous PSW
+;; and re-enable interrupts.  Because of that it always clobbers the CC_REG.
 (define_insn "mvtc"
   [(unspec_volatile:SI [(match_operand:SI 0 "immediate_operand" "i,i")
 	       (match_operand:SI 1 "nonmemory_operand" "r,i")]
-	      UNSPEC_BUILTIN_MVTC)]
+	      UNSPEC_BUILTIN_MVTC)
+   (clobber (reg:CC CC_REG))]
   ""
   "mvtc\t%1, %C0"
   [(set_attr "length" "3,7")]
-  ;; Ignore possible clobbering of the comparison flags in the
-  ;; PSW register.  This is a cc0 target so any cc0 setting
-  ;; instruction will always be paired with a cc0 user, without
-  ;; the possibility of this instruction being placed in between
-  ;; them.
 )
 
 ;; Move to interrupt priority level

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