Re: [RX] Add support for atomic operations
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
Re: [RX] Add support for atomic operations
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__fetchsi): Extract minus operator into ... > (atomic_sub_fetchsi): ... this new pattern. > (mvtc): Add CC_REG clobber. Approved - please apply. Cheers
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, atomic_exchangesi, xchg_mem,
> > atomic_fetch_si, atomic_fetch_nandsi,
> > atomic__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__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__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__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
Re: [RX] Add support for atomic operations
Hi Oleg, > 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, atomic_exchangesi, xchg_mem, > atomic_fetch_si, atomic_fetch_nandsi, > atomic__fetchsi, atomic_nand_fetchsi): New patterns. Approved - please apply. Cheers Nick
[RX] Add support for atomic operations
Hi,
The attached patch adds some rudimentary support for atomic operations.
On the original RX there is one truly atomic insn "xchg". All other
operations have to implement atomicity in some other way. One straight
forward option which is already being done on SH is to disable
interrupts for the duration of the atomic sequence. This works for
single-core systems that run in privileged mode. And that's what the
patch does.
OK for trunk?
Cheers,
Oleg
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, atomic_exchangesi, xchg_mem,
atomic_fetch_si, atomic_fetch_nandsi,
atomic__fetchsi, atomic_nand_fetchsi): New patterns.Index: gcc/config/rx/rx-protos.h
===
--- gcc/config/rx/rx-protos.h (revision 235992)
+++ gcc/config/rx/rx-protos.h (working copy)
@@ -26,6 +26,28 @@
extern void rx_expand_prologue (void);
extern int rx_initial_elimination_offset (int, int);
+bool is_interrupt_func (const_tree decl);
+bool is_fast_interrupt_func (const_tree decl);
+
+/* rx_atomic_sequence is used to emit the header and footer
+ of an atomic sequence. It's supposed to be used in a scope.
+ When constructed, it will emit the atomic sequence header insns.
+ When destructred (goes out of scope), it will emit the
+ corresponding atomic sequence footer insns. */
+class rx_atomic_sequence
+{
+public:
+ rx_atomic_sequence (const_tree fun_decl);
+ ~rx_atomic_sequence (void);
+
+private:
+ rx_atomic_sequence (void);
+ rx_atomic_sequence (const rx_atomic_sequence&);
+ rx_atomic_sequence& operator = (const rx_atomic_sequence&);
+
+ rtx m_prev_psw_reg;
+};
+
#ifdef RTX_CODE
extern int rx_adjust_insn_length (rtx_insn *, int);
extern int rx_align_for_label (rtx, int);
Index: gcc/config/rx/rx.c
===
--- gcc/config/rx/rx.c (revision 235992)
+++ gcc/config/rx/rx.c (working copy)
@@ -630,15 +630,15 @@
gcc_assert (CONST_INT_P (op));
switch (INTVAL (op))
{
- case 0: fprintf (file, "psw"); break;
- case 2: fprintf (file, "usp"); break;
- case 3: fprintf (file, "fpsw"); break;
- case 4: fprintf (file, "cpen"); break;
- case 8: fprintf (file, "bpsw"); break;
- case 9: fprintf (file, "bpc"); break;
- case 0xa: fprintf (file, "isp"); break;
- case 0xb: fprintf (file, "fintv"); break;
- case 0xc: fprintf (file, "intb"); break;
+ case CTRLREG_PSW: fprintf (file, "psw"); break;
+ case CTRLREG_USP: fprintf (file, "usp"); break;
+ case CTRLREG_FPSW: fprintf (file, "fpsw"); break;
+ case CTRLREG_CPEN: fprintf (file, "cpen"); break;
+ case CTRLREG_BPSW: fprintf (file, "bpsw"); break;
+ case CTRLREG_BPC: fprintf (file, "bpc"); break;
+ case CTRLREG_ISP: fprintf (file, "isp"); break;
+ case CTRLREG_FINTV: fprintf (file, "fintv"); break;
+ case CTRLREG_INTB: fprintf (file, "intb"); break;
default:
warning (0, "unrecognized control register number: %d - using 'psw'",
(int) INTVAL (op));
@@ -1216,7 +1216,7 @@
/* Returns true if the provided function has the "fast_interrupt" attribute. */
-static inline bool
+bool
is_fast_interrupt_func (const_tree decl)
{
return has_func_attr (decl, "fast_interrupt");
@@ -1224,7 +1224,7 @@
/* Returns true if the provided function has the "interrupt" attribute. */
-static inline bool
+bool
is_interrupt_func (const_tree decl)
{
return has_func_attr (decl, "interrupt");
@@ -3409,6 +3409,29 @@
return TARGET_ENABLE_LRA;
}
+rx_atomic_sequence::rx_atomic_sequence (const_tree fun_decl)
+{
+ if (is_fast_interrupt_func (fun_decl) || is_interrupt_func (fun_decl))
+{
+ /* If we are inside an interrupt handler, assume that interrupts are
+ off -- which is the default hardware behavior. In this case, there
+ is no need to disable the interrupts. */
+ m_prev_psw_reg = NULL;
+}
+ else
+{
+ m_prev_psw_reg = gen_reg_rtx (SImode);
+ emit_insn (gen_mvfc (m_prev_psw_reg, GEN_INT (CTRLREG_PSW)));
+ emit_insn (gen_clrpsw (GEN_INT ('I')));
+}
+}
+
+rx_atomic_sequence::~rx_atomic_sequence (void)
+{
+ if (m_prev_psw_reg != NULL)
+emit_insn (gen_mvtc (GEN_INT (CTRLREG_PSW), m_prev_psw_reg));
+}
+
#undef
