diff mbox

[RX] Add support for atomic operations

Message ID 1464491872.2269.93.camel@t-online.de
State New
Headers show

Commit Message

Oleg Endo May 29, 2016, 3:17 a.m. UTC 
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.

Comments

Nick Clifton May 31, 2016, 1:17 p.m. UTC | #1 
Hi Oleg,

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

Thanks for your diligence in checking the patch.

> 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.

Approved - please apply.

Cheers
Oleg Endo May 31, 2016, 3:08 p.m. UTC | #2 
On Tue, 2016-05-31 at 14:17 +0100, Nick Clifton wrote:

> > Sorry, but my original patch was buggy.  There are two problems:
> 
> Thanks for your diligence in checking the patch.

Just eating my own dogfood here ... :)

> Approved - please apply.

Committed as r236926.

Cheers,
Oleg
diff mbox

Patch

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