[RFC PATCH 8/9] [SH] Add splitter to addsi3_compact

[RFC PATCH 8/9] [SH] Add splitter to addsi3_compact

[Kaz Kojima]

This patch is discussed in PR55212

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55212#c65

and is to make LRA's register elimination work well on SH.
The problem is SH has very limited add instructions only and expands
rA := rB + N to (set rA (const_int N)) and (set rA (plus rA rB))
instead of (set rA (plus rB (const_int N))).  It seems that the former
combination isn't good for LRA's register elimination pass.  The patch
adds splitter to addsi3_compact insn so that LRA can see the latter
rtl.

--
	* config/sh/predicates.md (arith_or_int_operand): New predicate.
	* config/sh/sh.md (addsi3): Use arith_or_int_operand for operand 2.
	Return fail if operands[0] and operands[1] are overlap when
	operands[2] is integer constant.
	(*addsi3_compact): Make it define_insn_and_split which splits
	reg0 := reg1 + constant to reg0 = constant and reg0 := reg0 + reg1.

diff --git a/config/sh/predicates.md b/config/sh/predicates.md
index 152056a..8772332 100644
--- a/config/sh/predicates.md
+++ b/config/sh/predicates.md
@@ -182,6 +182,19 @@
   return 0;
 })
 
+;; Likewise arith_operand but always permits const_int.
+(define_predicate "arith_or_int_operand"
+  (match_code "subreg,reg,const_int,const_vector")
+{
+  if (arith_operand (op, mode))
+    return 1;
+
+  if (CONST_INT_P (op))
+    return 1;
+
+  return 0;
+})
+
 ;; Returns 1 if OP is a valid source operand for a compare insn.
 (define_predicate "arith_reg_or_0_operand"
   (match_code "subreg,reg,const_int,const_vector")
diff --git a/config/sh/sh.md b/config/sh/sh.md
index 6bc0084..bcc569d 100644
--- a/config/sh/sh.md
+++ b/config/sh/sh.md
@@ -2061,11 +2061,16 @@
 (define_expand "addsi3"
   [(set (match_operand:SI 0 "arith_reg_operand" "")
 	(plus:SI (match_operand:SI 1 "arith_operand" "")
-		 (match_operand:SI 2 "arith_operand" "")))]
+		 (match_operand:SI 2 "arith_or_int_operand" "")))]
   ""
 {
   if (TARGET_SHMEDIA)
     operands[1] = force_reg (SImode, operands[1]);
+  else if (! arith_operand (operands[2], SImode))
+    {
+      if (reg_overlap_mentioned_p (operands[0], operands[1]))
+	FAIL;
+    }
 })
 
 (define_insn "addsi3_media"
@@ -2092,12 +2097,22 @@
   [(set_attr "type" "arith_media")
    (set_attr "highpart" "ignore")])
 
-(define_insn "*addsi3_compact"
-  [(set (match_operand:SI 0 "arith_reg_dest" "=r")
-	(plus:SI (match_operand:SI 1 "arith_operand" "%0")
-		 (match_operand:SI 2 "arith_operand" "rI08")))]
-  "TARGET_SH1"
-  "add	%2,%0"
+(define_insn_and_split "*addsi3_compact"
+  [(set (match_operand:SI 0 "arith_reg_dest" "=r,&r")
+	(plus:SI (match_operand:SI 1 "arith_operand" "%0,r")
+		 (match_operand:SI 2 "arith_or_int_operand" "rI08,rn")))]
+  "TARGET_SH1
+   && (rtx_equal_p (operands[0], operands[1])
+       && arith_operand (operands[2], SImode))
+      || ! reg_overlap_mentioned_p (operands[0], operands[1])"
+  "@
+	add	%2,%0
+	#"
+  "reload_completed
+   && ! reg_overlap_mentioned_p (operands[0], operands[1])"
+  [(set (match_dup 0) (match_dup 2))
+   (set (match_dup 0) (plus:SI (match_dup 0) (match_dup 1)))]
+  ""
   [(set_attr "type" "arith")])
 
 ;; -------------------------------------------------------------------------

Re: [RFC PATCH 8/9] [SH] Add splitter to addsi3_compact

[Oleg Endo]

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

On Thu, 2014-12-18 at 10:04 +0900, Kaz Kojima wrote:
> This patch is discussed in PR55212
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55212#c65
> 
> and is to make LRA's register elimination work well on SH.
> The problem is SH has very limited add instructions only and expands
> rA := rB + N to (set rA (const_int N)) and (set rA (plus rA rB))
> instead of (set rA (plus rB (const_int N))).  It seems that the former
> combination isn't good for LRA's register elimination pass.  The patch
> adds splitter to addsi3_compact insn so that LRA can see the latter
> rtl.
> 
> --
> 	* config/sh/predicates.md (arith_or_int_operand): New predicate.
> 	* config/sh/sh.md (addsi3): Use arith_or_int_operand for operand 2.
> 	Return fail if operands[0] and operands[1] are overlap when
> 	operands[2] is integer constant.
> 	(*addsi3_compact): Make it define_insn_and_split which splits
> 	reg0 := reg1 + constant to reg0 = constant and reg0 := reg0 + reg1.

This triggered a warning about suggested parentheses around the &&
condition in *addsi3_compact.  I've committed the attached patch as
r218999.

Cheers,
Oleg

gcc/ChangeLog:
	PR target/55212
	* config/sh/sh.md (*addsi3_compact): Add parentheses around &&
	condition.  Add comments.


[-- Attachment #2: sh_pr55212_addsi3_compact.patch --]
[-- Type: text/x-patch, Size: 1203 bytes --]

Index: gcc/config/sh/sh.md
===================================================================
--- gcc/config/sh/sh.md	(revision 218997)
+++ gcc/config/sh/sh.md	(working copy)
@@ -2056,14 +2056,20 @@
   [(set_attr "type" "arith_media")
    (set_attr "highpart" "ignore")])
 
+;; The *addsi3_compact is made an insn_and_split and accepts actually
+;; impossible constraints to make LRA's register elimination work well on SH.
+;; The problem is that LRA expects something like
+;;    (set rA (plus rB (const_int N)))
+;; to work.  We can do that, but we have to split out an additional reg-reg
+;; copy before the actual add insn.
 (define_insn_and_split "*addsi3_compact"
   [(set (match_operand:SI 0 "arith_reg_dest" "=r,&r")
 	(plus:SI (match_operand:SI 1 "arith_operand" "%0,r")
 		 (match_operand:SI 2 "arith_or_int_operand" "rI08,rn")))]
   "TARGET_SH1
-   && (rtx_equal_p (operands[0], operands[1])
-       && arith_operand (operands[2], SImode))
-      || ! reg_overlap_mentioned_p (operands[0], operands[1])"
+   && ((rtx_equal_p (operands[0], operands[1])
+        && arith_operand (operands[2], SImode))
+       || ! reg_overlap_mentioned_p (operands[0], operands[1]))"
   "@
 	add	%2,%0
 	#"

Re: [RFC PATCH 8/9] [SH] Add splitter to addsi3_compact

[Oleg Endo]

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

On Thu, 2014-12-18 at 10:04 +0900, Kaz Kojima wrote:
> This patch is discussed in PR55212
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55212#c65
> 
> and is to make LRA's register elimination work well on SH.
> The problem is SH has very limited add instructions only and expands
> rA := rB + N to (set rA (const_int N)) and (set rA (plus rA rB))
> instead of (set rA (plus rB (const_int N))).  It seems that the former
> combination isn't good for LRA's register elimination pass.  The patch
> adds splitter to addsi3_compact insn so that LRA can see the latter
> rtl.
> 
> --
> 	* config/sh/predicates.md (arith_or_int_operand): New predicate.
> 	* config/sh/sh.md (addsi3): Use arith_or_int_operand for operand 2.
> 	Return fail if operands[0] and operands[1] are overlap when
> 	operands[2] is integer constant.
> 	(*addsi3_compact): Make it define_insn_and_split which splits
> 	reg0 := reg1 + constant to reg0 = constant and reg0 := reg0 + reg1.

I've noticed that this change tends to produce code like ...
	mov	#123,rA
	...
	add	rA,rB

It's a bit better to do:
	mov	rB,rA
	...
	add	#123,rA

if the constant fits into #imm8.  The attached patch does that.
Tested with make -k check RUNTESTFLAGS="--target_board=sh-sim
\{-m2/-ml,-m2/-mb,-m2a/-mb,-m4/-ml,-m4/-mb,-m4a/-ml,-m4a/-mb}"
and no new failures.  CSiBE shows a couple of cases where code size
decreases.  Committed as r219341.

Cheers,
Oleg

gcc/ChangeLog:
	PR target/55212
	* config/sh/sh.md (*addsi3_compact): Emit reg-reg copy instead of
	constant load if constant operand fits into I08.


[-- Attachment #2: sh_pr55212_addsi3_compact_2.patch --]
[-- Type: text/x-patch, Size: 1255 bytes --]

Index: gcc/config/sh/sh.md
===================================================================
--- gcc/config/sh/sh.md	(revision 219339)
+++ gcc/config/sh/sh.md	(working copy)
@@ -2061,8 +2061,9 @@
 ;; The problem is that LRA expects something like
 ;;    (set rA (plus rB (const_int N)))
 ;; to work.  We can do that, but we have to split out an additional reg-reg
-;; copy before the actual add insn.  Use u constraint for that case to avoid
-;; the invalid value in the stack pointer.
+;; copy or constant load before the actual add insn.
+;; Use u constraint for that case to avoid the invalid value in the stack
+;; pointer.
 (define_insn_and_split "*addsi3_compact"
   [(set (match_operand:SI 0 "arith_reg_dest" "=r,&u")
 	(plus:SI (match_operand:SI 1 "arith_operand" "%0,r")
@@ -2078,7 +2079,11 @@
    && ! reg_overlap_mentioned_p (operands[0], operands[1])"
   [(set (match_dup 0) (match_dup 2))
    (set (match_dup 0) (plus:SI (match_dup 0) (match_dup 1)))]
-  ""
+{
+  /* Prefer 'mov r0,r1; add #imm8,r1' over 'mov #imm8,r1; add r0,r1'  */
+  if (satisfies_constraint_I08 (operands[2]))
+    std::swap (operands[1], operands[2]);
+}
   [(set_attr "type" "arith")])
 
 ;; -------------------------------------------------------------------------