Patchwork [SH] Fix PR 58314 - Rework *movqi / *movhi patterns

login
register
mail settings
Submitter Oleg Endo
Date Dec. 6, 2013, 7:38 p.m.
Message ID <1386358711.14008.98.camel@yam-132-YW-E178-FTW>
Download mbox | patch
Permalink /patch/298233/
State New
Headers show

Comments

Oleg Endo - Dec. 6, 2013, 7:38 p.m.
On Tue, 2013-11-26 at 12:18 +0100, Oleg Endo wrote:
> Hello,
> 
> The attached patch is the same as posted in the PR as attachment 31283.
> In addition to the testing done by Kaz and Christian, I've also tested
> it with
> make -k check RUNTESTFLAGS="--target_board=sh-sim
> \{-m2/-ml,-m2/-mb,-m2a/-mb,-m4/-ml,-m4/-mb,-m4a/-ml,-m4a/-mb}"
> 
> on rev 205313 with no new failures.
> OK for trunk?
> 
> The additional test case from PR 58314 will follow.
> I'll also post a version of the patch for 4.8 after testing.

I've just committed the attached patch to the 4.8 branch as rev 205759.
Tested with
make -k check RUNTESTFLAGS="--target_board=sh-sim
\{-m2/-ml,-m2/-mb,-m2a/-mb,-m4/-ml,-m4/-mb,-m4a/-ml,-m4a/-mb}"

Cheers,
Oleg

gcc/ChangeLog:
	Backport from mainline
	2013-11-26  Oleg Endo  <olegendo@gcc.gnu.org>

	PR target/58314
	PR target/50751
	* config/sh/sh.c (max_mov_insn_displacement, disp_addr_displacement):
	Prefix function names with 'sh_'.  Make them non-static.
	* config/sh/sh-protos.h (sh_disp_addr_displacement,
	sh_max_mov_insn_displacement): Add declarations.
	* config/sh/constraints.md (Q): Reject QImode.
	(Sdd): Use match_code "mem".
	(Snd): Fix erroneous matching of non-memory operands.
	* config/sh/predicates.md (short_displacement_mem_operand): New
	predicate.
	(general_movsrc_operand): Disallow PC relative QImode loads.
	* config/sh/sh.md (*mov<mode>_reg_reg): Remove it.
	(*movqi, *movhi): Merge both insns into...
	(*mov<mode>): ... this new insn.  Replace generic 'm' constraints with
	'Snd' and 'Sdd' constraints.  Calculate insn length dynamically based
	on the operand types.

Patch

Index: gcc/config/sh/constraints.md
===================================================================
--- gcc/config/sh/constraints.md	(revision 205733)
+++ gcc/config/sh/constraints.md	(working copy)
@@ -221,6 +221,7 @@ 
 (define_constraint "Q"
   "A pc relative load operand."
   (and (match_code "mem")
+       (match_test "GET_MODE (op) != QImode")
        (match_test "IS_PC_RELATIVE_LOAD_ADDR_P (XEXP (op, 0))")))
 
 (define_constraint "Bsc"
@@ -295,13 +296,15 @@ 
 
 (define_memory_constraint "Sdd"
   "A memory reference that uses displacement addressing."
-  (and (match_test "MEM_P (op) && GET_CODE (XEXP (op, 0)) == PLUS")
+  (and (match_code "mem")
+       (match_test "GET_CODE (XEXP (op, 0)) == PLUS")
        (match_test "REG_P (XEXP (XEXP (op, 0), 0))")
        (match_test "CONST_INT_P (XEXP (XEXP (op, 0), 1))")))
 
 (define_memory_constraint "Snd"
   "A memory reference that excludes displacement addressing."
-  (match_test "! satisfies_constraint_Sdd (op)"))
+  (and (match_code "mem")
+       (match_test "! satisfies_constraint_Sdd (op)")))
 
 (define_memory_constraint "Sbv"
   "A memory reference, as used in SH2A bclr.b, bset.b, etc."
Index: gcc/config/sh/sh.md
===================================================================
--- gcc/config/sh/sh.md	(revision 205734)
+++ gcc/config/sh/sh.md	(working copy)
@@ -6831,34 +6831,9 @@ 
   prepare_move_operands (operands, QImode);
 })
 
-;; If movqi_reg_reg is specified as an alternative of movqi, movqi will be
-;; selected to copy QImode regs.  If one of them happens to be allocated
-;; on the stack, reload will stick to movqi insn and generate wrong
-;; displacement addressing because of the generic m alternatives.
-;; With the movqi_reg_reg being specified before movqi it will be initially
-;; picked to load/store regs.  If the regs regs are on the stack reload
-;; try other insns and not stick to movqi_reg_reg, unless there were spilled
-;; pseudos in which case 'm' constraints pertain.
-;; The same applies to the movhi variants.
-;;
-;; Notice, that T bit is not allowed as a mov src operand here.  This is to
-;; avoid things like (set (reg:QI) (subreg:QI (reg:SI T_REG) 0)), which
-;; introduces zero extensions after T bit stores and redundant reg copies.
-;;
-;; FIXME: We can't use 'arith_reg_operand' (which disallows T_REG) as a
-;; predicate for the mov src operand because reload will have trouble
-;; reloading MAC subregs otherwise.  For that probably special patterns
-;; would be required.
-(define_insn "*mov<mode>_reg_reg"
-  [(set (match_operand:QIHI 0 "arith_reg_dest" "=r,m,*z")
-	(match_operand:QIHI 1 "register_operand" "r,*z,m"))]
-  "TARGET_SH1 && !t_reg_operand (operands[1], VOIDmode)"
-  "@
-    mov		%1,%0
-    mov.<bw>	%1,%0
-    mov.<bw>	%1,%0"
-  [(set_attr "type" "move,store,load")])
-
+;; Specifying the displacement addressing load / store patterns separately
+;; before the generic movqi / movhi pattern allows controlling the order
+;; in which load / store insns are selected in a more fine grained way.
 ;; FIXME: The non-SH2A and SH2A variants should be combined by adding
 ;; "enabled" attribute as it is done in other targets.
 (define_insn "*mov<mode>_store_mem_disp04"
@@ -6908,38 +6883,44 @@ 
   [(set_attr "type" "load")
    (set_attr "length" "2,2,4")])
 
-;; The m constraints basically allow any kind of addresses to be used with any
-;; source/target register as the other operand.  This is not true for 
-;; displacement addressing modes on anything but SH2A.  That's why the
-;; specialized load/store insns are specified above.
-(define_insn "*movqi"
-  [(set (match_operand:QI 0 "general_movdst_operand" "=r,r,m,r,l")
-	(match_operand:QI 1 "general_movsrc_operand"  "i,m,r,l,r"))]
+;; The order of the constraint alternatives is important here.
+;; Q/r has to come first, otherwise PC relative loads might wrongly get
+;; placed into delay slots.  Since there is no QImode PC relative load, the
+;; Q constraint and general_movsrc_operand will reject it for QImode.
+;; The Snd alternatives should come before Sdd in order to avoid a preference
+;; of using r0 als the register operand for addressing modes other than
+;; displacement addressing.
+;; The Sdd alternatives allow only r0 as register operand, even though on
+;; SH2A any register could be allowed by switching to a 32 bit insn.
+;; Generally sticking to the r0 is preferrable, since it generates smaller
+;; code.  Obvious r0 reloads can then be eliminated with a peephole on SH2A.
+(define_insn "*mov<mode>"
+  [(set (match_operand:QIHI 0 "general_movdst_operand"
+			      "=r,r,r,Snd,r,  Sdd,z,  r,l")
+	(match_operand:QIHI 1 "general_movsrc_operand"
+			       "Q,r,i,r,  Snd,z,  Sdd,l,r"))]
   "TARGET_SH1
-   && (arith_reg_operand (operands[0], QImode)
-       || arith_reg_operand (operands[1], QImode))"
+   && (arith_reg_operand (operands[0], <MODE>mode)
+       || arith_reg_operand (operands[1], <MODE>mode))"
   "@
+	mov.<bw>	%1,%0
 	mov	%1,%0
-	mov.b	%1,%0
-	mov.b	%1,%0
-	sts	%1,%0
-	lds	%1,%0"
- [(set_attr "type" "movi8,load,store,prget,prset")])
-
-(define_insn "*movhi"
-  [(set (match_operand:HI 0 "general_movdst_operand" "=r,r,r,m,r,l")
-	(match_operand:HI 1 "general_movsrc_operand"  "Q,i,m,r,l,r"))]
-  "TARGET_SH1
-   && (arith_reg_operand (operands[0], HImode)
-       || arith_reg_operand (operands[1], HImode))"
-  "@
-	mov.w	%1,%0
 	mov	%1,%0
-	mov.w	%1,%0
-	mov.w	%1,%0
+	mov.<bw>	%1,%0
+	mov.<bw>	%1,%0
+	mov.<bw>	%1,%0
+	mov.<bw>	%1,%0
 	sts	%1,%0
 	lds	%1,%0"
- [(set_attr "type" "pcload,movi8,load,store,prget,prset")])
+  [(set_attr "type" "pcload,move,movi8,store,load,store,load,prget,prset")
+   (set (attr "length")
+	(cond [(and (match_operand 0 "displacement_mem_operand")
+		    (not (match_operand 0 "short_displacement_mem_operand")))
+	       (const_int 4)
+	       (and (match_operand 1 "displacement_mem_operand")
+		    (not (match_operand 1 "short_displacement_mem_operand")))
+	       (const_int 4)]
+	      (const_int 2)))])
 
 (define_insn "*movqi_media"
   [(set (match_operand:QI 0 "general_movdst_operand" "=r,r,r,m")
Index: gcc/config/sh/predicates.md
===================================================================
--- gcc/config/sh/predicates.md	(revision 205733)
+++ gcc/config/sh/predicates.md	(working copy)
@@ -389,6 +389,12 @@ 
 					   XEXP (XEXP (op, 0), 1),
 					   TARGET_SH2A, true)")))
 
+;; Returns true if OP is a displacement address that can fit into a
+;; 16 bit (non-SH2A) memory load / store insn.
+(define_predicate "short_displacement_mem_operand"
+  (match_test "sh_disp_addr_displacement (op)
+	       <= sh_max_mov_insn_displacement (GET_MODE (op), false)"))
+
 ;; Returns 1 if the operand can be used in an SH2A movu.{b|w} insn.
 (define_predicate "zero_extend_movu_operand"
   (and (match_operand 0 "displacement_mem_operand")
@@ -413,6 +419,11 @@ 
   if (t_reg_operand (op, mode))
     return 0;
 
+  /* Disallow PC relative QImode loads, since these is no insn to do that
+     and an imm8 load should be used instead.  */
+  if (IS_PC_RELATIVE_LOAD_ADDR_P (op) && GET_MODE (op) == QImode)
+    return false;
+
   if (MEM_P (op))
     {
       rtx inside = XEXP (op, 0);
Index: gcc/config/sh/sh-protos.h
===================================================================
--- gcc/config/sh/sh-protos.h	(revision 205733)
+++ gcc/config/sh/sh-protos.h	(working copy)
@@ -159,6 +159,8 @@ 
 extern bool sh_cfun_trap_exit_p (void);
 extern rtx sh_find_equiv_gbr_addr (rtx cur_insn, rtx mem);
 extern int sh_eval_treg_value (rtx op);
+extern HOST_WIDE_INT sh_disp_addr_displacement (rtx mem_op);
+extern int sh_max_mov_insn_displacement (machine_mode mode, bool consider_sh2a);
 
 /* Result value of sh_find_set_of_reg.  */
 struct set_of_reg
Index: gcc/config/sh/sh.c
===================================================================
--- gcc/config/sh/sh.c	(revision 205733)
+++ gcc/config/sh/sh.c	(working copy)
@@ -310,9 +310,7 @@ 
 static void sh_conditional_register_usage (void);
 static bool sh_legitimate_constant_p (enum machine_mode, rtx);
 static int mov_insn_size (enum machine_mode, bool);
-static int max_mov_insn_displacement (enum machine_mode, bool);
 static int mov_insn_alignment_mask (enum machine_mode, bool);
-static HOST_WIDE_INT disp_addr_displacement (rtx);
 static bool sequence_insn_p (rtx);
 static void sh_canonicalize_comparison (int *, rtx *, rtx *, bool);
 static void sh_canonicalize_comparison (enum rtx_code&, rtx&, rtx&,
@@ -3628,8 +3626,8 @@ 
 
 /* Determine the maximum possible displacement for a move insn for the
    specified mode.  */
-static int
-max_mov_insn_displacement (enum machine_mode mode, bool consider_sh2a)
+int
+sh_max_mov_insn_displacement (machine_mode mode, bool consider_sh2a)
 {
   /* The 4 byte displacement move insns are the same as the 2 byte
      versions but take a 12 bit displacement.  All we need to do is to
@@ -3665,8 +3663,8 @@ 
 }
 
 /* Return the displacement value of a displacement address.  */
-static inline HOST_WIDE_INT
-disp_addr_displacement (rtx x)
+HOST_WIDE_INT
+sh_disp_addr_displacement (rtx x)
 {
   gcc_assert (satisfies_constraint_Sdd (x));
   return INTVAL (XEXP (XEXP (x, 0), 1));
@@ -3703,12 +3701,12 @@ 
 	 HImode and QImode loads/stores with displacement put pressure on
 	 R0 which will most likely require another reg copy.  Thus account
 	 a higher cost for that.  */
-      if (offset > 0 && offset <= max_mov_insn_displacement (mode, false))
+      if (offset > 0 && offset <= sh_max_mov_insn_displacement (mode, false))
 	return (mode == HImode || mode == QImode) ? 2 : 1;
 
       /* The displacement would fit into a 4 byte move insn (SH2A).  */
       if (TARGET_SH2A
-	  && offset > 0 && offset <= max_mov_insn_displacement (mode, true))
+	  && offset > 0 && offset <= sh_max_mov_insn_displacement (mode, true))
 	return 2;
 
       /* The displacement is probably out of range and will require extra
@@ -10218,7 +10216,7 @@ 
   else
     {
       const HOST_WIDE_INT offset = INTVAL (op);
-      const int max_disp = max_mov_insn_displacement (mode, consider_sh2a);
+      const int max_disp = sh_max_mov_insn_displacement (mode, consider_sh2a);
       const int align_mask = mov_insn_alignment_mask (mode, consider_sh2a);
 
       /* If the mode does not support any displacement always return false.
@@ -10404,7 +10402,7 @@ 
      effectively disable the small displacement insns.  */
   const int mode_sz = GET_MODE_SIZE (mode);
   const int mov_insn_sz = mov_insn_size (mode, false);
-  const int max_disp = max_mov_insn_displacement (mode, false);
+  const int max_disp = sh_max_mov_insn_displacement (mode, false);
   const int max_disp_next = max_disp + mov_insn_sz;
   HOST_WIDE_INT align_modifier = offset > 127 ? mov_insn_sz : 0;
   HOST_WIDE_INT offset_adjust;
@@ -13165,7 +13163,8 @@ 
      the insns must have the appropriate alternatives.  */
   if ((mode == QImode || mode == HImode) && rclass != R0_REGS
       && satisfies_constraint_Sdd (x)
-      && disp_addr_displacement (x) <= max_mov_insn_displacement (mode, false))
+      && sh_disp_addr_displacement (x)
+	 <= sh_max_mov_insn_displacement (mode, false))
     return R0_REGS;
 
   /* When reload is trying to address a QImode or HImode subreg on the stack,