diff mbox

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

Message ID 1385464704.5048.26.camel@yam-132-YW-E178-FTW
State New
Headers show

Commit Message

Oleg Endo Nov. 26, 2013, 11:18 a.m. UTC
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.

Cheers,
Oleg

gcc/ChangeLog:
	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' 
	constraint with 'Snd' and 'Sdd' constraints.  Calculate insn 
	length dynamically based on the operand types.

Comments

Kaz Kojima Nov. 26, 2013, 11:38 a.m. UTC | #1
Oleg Endo <oleg.endo@t-online.de> wrote:
> 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?

OK.

Regards,
	kaz
diff mbox

Patch

Index: gcc/config/sh/constraints.md
===================================================================
--- gcc/config/sh/constraints.md	(revision 205190)
+++ 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 205190)
+++ gcc/config/sh/sh.md	(working copy)
@@ -6993,34 +6993,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"
@@ -7070,38 +7045,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 205190)
+++ gcc/config/sh/predicates.md	(working copy)
@@ -421,6 +421,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")
@@ -445,6 +451,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 205190)
+++ gcc/config/sh/sh-protos.h	(working copy)
@@ -162,6 +162,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 205191)
+++ gcc/config/sh/sh.c	(working copy)
@@ -308,9 +308,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&,
@@ -3574,8 +3572,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
@@ -3611,8 +3609,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));
@@ -3649,12 +3647,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
@@ -10167,7 +10165,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.
@@ -10353,7 +10351,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;
@@ -13113,7 +13111,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,