[PATCH] Provide a hook for target to disable register renaming for some instructions

[PATCH] Provide a hook for target to disable register renaming for some instructions

[Jie Zhang]

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

When compiler an application for ARM, the GAS issued a warning:

Warning: register range not in ascending order

for the instruction

push	{ip, r3, r4, lr}

Before regrename pass, this instruction looked like

push	{r0, r3, r4, lr}

Then GCC renamed r0 to ip in the regrename pass.  Such kind of register 
renaming is bad since the resulted register list is not in ascending 
order any more. And each register in the list was pushed at a wrong 
memory address.

This patch adds a target hook to provide a way for target to disable 
register renaming in such case when necessary.

Bootstrap and regression tested on x86_64. Regression tested for ARM 
NEON on arm-none-eabi target. Both are OK.

I have no small test case can be included. :-(

Is it OK?


Regards,
-- 
Jie Zhang
CodeSourcery




[-- Attachment #2: gcc-regrename-ok-hook-5.diff --]
[-- Type: text/x-patch, Size: 6503 bytes --]


	* regrename.c: Include "target.h".
	(regrename_optimize): Call targetm.regrename_ok.
	* targhooks.c (default_regrename_ok): New.
	* targhooks.h (default_regrename_ok): Declare.
	* target.def (regrename_ok): New.
	* doc/tm.texi.in: Document TARGET_REGRENAME_OK.
	* doc/tm.texi: Regenerate.
	* config/arm/arm.c (arm_regrename_ok): New.
	(TARGET_REGRENAME_OK): Define.
	* config/arm/arm.md (*push_multi): Rename to push_multi.

Index: regrename.c
===================================================================
--- regrename.c	(revision 161569)
+++ regrename.c	(working copy)
@@ -36,6 +36,7 @@
 #include "flags.h"
 #include "toplev.h"
 #include "obstack.h"
+#include "target.h"
 #include "timevar.h"
 #include "tree-pass.h"
 #include "df.h"
@@ -301,7 +302,8 @@ regrename_optimize (void)
 			&& ! (HARD_REGNO_CALL_PART_CLOBBERED
 			      (reg, GET_MODE (*tmp->loc)))
 			&& (HARD_REGNO_CALL_PART_CLOBBERED
-			    (new_reg, GET_MODE (*tmp->loc)))))
+			    (new_reg, GET_MODE (*tmp->loc))))
+		    || ! targetm.regrename_ok (tmp->insn, reg, new_reg))
 		  break;
 	      if (! tmp)
 		{
Index: doc/tm.texi.in
===================================================================
--- doc/tm.texi.in	(revision 161569)
+++ doc/tm.texi.in	(working copy)
@@ -2256,6 +2256,18 @@ is not saved by a prologue in an interru
 The default version of this hook always returns @code{true}.
 @end deftypefn
 
+@hook TARGET_REGRENAME_OK
+
+This target hook should return @code{true} if it is OK to rename a register
+@var{reg} to a new register @var{new_reg}.
+
+This macro can be used to prevent register renaming in some cases.  ARM port uses
+this macro to prevent register renaming generating non-ascending register list
+in push multiple instruction.
+
+The default version of this hook always returns @code{true}.
+@end deftypefn
+
 @defmac AVOID_CCMODE_COPIES
 Define this macro if the compiler should avoid copies to/from @code{CCmode}
 registers.  You should only define this macro if support for copying to/from
Index: targhooks.c
===================================================================
--- targhooks.c	(revision 161569)
+++ targhooks.c	(working copy)
@@ -1102,6 +1102,16 @@ default_mode_dependent_address_p (const_
 #endif
 }
 
+/* The default implementation of TARGET_REGRENAME_OK.  */
+
+bool
+default_regrename_ok (rtx insn ATTRIBUTE_UNUSED,
+		      int reg ATTRIBUTE_UNUSED,
+		      int new_reg ATTRIBUTE_UNUSED)
+{
+  return true;
+}
+
 bool
 default_target_option_valid_attribute_p (tree ARG_UNUSED (fndecl),
 					 tree ARG_UNUSED (name),
Index: targhooks.h
===================================================================
--- targhooks.h	(revision 161569)
+++ targhooks.h	(working copy)
@@ -130,6 +130,7 @@ extern tree default_mangle_decl_assemble
 extern tree default_emutls_var_fields (tree, tree *);
 extern tree default_emutls_var_init (tree, tree, tree);
 extern bool default_hard_regno_scratch_ok (unsigned int);
+extern bool default_regrename_ok (rtx, int, int);
 extern bool default_mode_dependent_address_p (const_rtx addr);
 extern bool default_target_option_valid_attribute_p (tree, tree, tree, int);
 extern bool default_target_option_pragma_parse (tree, tree);
Index: target.def
===================================================================
--- target.def	(revision 161569)
+++ target.def	(working copy)
@@ -1980,6 +1980,13 @@ DEFHOOK
  bool, (unsigned int regno),
  default_hard_regno_scratch_ok)
 
+/* Return true if is OK to rename REG to NEW_REG in INSN.  */
+DEFHOOK
+(regrename_ok,
+ "",
+ bool, (rtx insn, int reg, int new_reg),
+ default_regrename_ok)
+
 /* Return the smallest number of different values for which it is best to
    use a jump-table instead of a tree of conditional branches.  */
 DEFHOOK
Index: config/arm/arm.c
===================================================================
--- config/arm/arm.c	(revision 161569)
+++ config/arm/arm.c	(working copy)
@@ -219,6 +219,7 @@ static const char *arm_invalid_parameter
 static const char *arm_invalid_return_type (const_tree t);
 static tree arm_promoted_type (const_tree t);
 static tree arm_convert_to_type (tree type, tree expr);
+static bool arm_regrename_ok (rtx, int, int);
 static bool arm_scalar_mode_supported_p (enum machine_mode);
 static bool arm_frame_pointer_required (void);
 static bool arm_can_eliminate (const int, const int);
@@ -507,6 +508,9 @@ static const struct attribute_spec arm_a
 #undef TARGET_CONVERT_TO_TYPE
 #define TARGET_CONVERT_TO_TYPE arm_convert_to_type
 
+#undef TARGET_REGRENAME_OK
+#define TARGET_REGRENAME_OK arm_regrename_ok
+
 #undef TARGET_SCALAR_MODE_SUPPORTED_P
 #define TARGET_SCALAR_MODE_SUPPORTED_P arm_scalar_mode_supported_p
 
@@ -18118,6 +18122,43 @@ arm_convert_to_type (tree type, tree exp
   return NULL_TREE;
 }
 
+/* Implement TARGET_REGRENAME_OK.
+   We cannot rename REG to NEW_REG in INSN if INSN is a push_multi instruction
+   and the registers aren't in ascending order any more after renaming.  */
+
+static bool
+arm_regrename_ok (rtx insn, int reg, int new_reg)
+{
+  if (recog_memoized (insn) == CODE_FOR_push_multi)
+    {
+      int regs[MAX_RECOG_OPERANDS + 2];
+      int num_regs, i;
+
+      extract_insn (insn);
+
+      num_regs = XVECLEN (recog_data.operand[2], 0);
+
+      if (num_regs == 1)
+	return true;
+
+      regs[0] = -1;
+      regs[1] = REGNO (recog_data.operand[1]);
+      regs[num_regs + 1] = FIRST_PSEUDO_REGISTER;
+      for (i = 1; i < num_regs; i++)
+	regs[i + 1] = REGNO (XEXP (XVECEXP (recog_data.operand[2], 0, i), 0));
+
+      for (i = 1; i <= num_regs; i++)
+	if (reg == regs[i])
+	  {
+	    if (new_reg > regs[i - 1] && new_reg < regs[i + 1])
+	      return true;
+	    else
+	      return false;
+	  }
+    }
+  return true;
+}
+
 /* Implement TARGET_SCALAR_MODE_SUPPORTED_P.
    This simply adds HFmode as a supported mode; even though we don't
    implement arithmetic on this type directly, it's supported by
Index: config/arm/arm.md
===================================================================
--- config/arm/arm.md	(revision 161569)
+++ config/arm/arm.md	(working copy)
@@ -10868,7 +10868,7 @@
 ;; Push multiple registers to the stack.  Registers are in parallel (use ...)
 ;; expressions.  For simplicity, the first register is also in the unspec
 ;; part.
-(define_insn "*push_multi"
+(define_insn "push_multi"
   [(match_parallel 2 "multi_register_push"
     [(set (match_operand:BLK 0 "memory_operand" "=m")
 	  (unspec:BLK [(match_operand:SI 1 "s_register_operand" "r")]

Re: [PATCH] Provide a hook for target to disable register renaming for some instructions

[Richard Henderson]

On 06/30/2010 10:53 AM, Jie Zhang wrote:
> When compiler an application for ARM, the GAS issued a warning:
> 
> Warning: register range not in ascending order
> 
> for the instruction
> 
> push    {ip, r3, r4, lr}
> 
> Before regrename pass, this instruction looked like
> 
> push    {r0, r3, r4, lr}

Doesn't it work just as well to simply remove the register
constraint from the push_multi instruction?  Without that
the regrename pass won't get a register class for the 
operand and will leave it alone.


r~

Re: [PATCH] Provide a hook for target to disable register renaming for some instructions

[Jie Zhang]

On 07/01/2010 02:03 AM, Richard Henderson wrote:
> On 06/30/2010 10:53 AM, Jie Zhang wrote:
>> When compiler an application for ARM, the GAS issued a warning:
>>
>> Warning: register range not in ascending order
>>
>> for the instruction
>>
>> push    {ip, r3, r4, lr}
>>
>> Before regrename pass, this instruction looked like
>>
>> push    {r0, r3, r4, lr}
>
> Doesn't it work just as well to simply remove the register
> constraint from the push_multi instruction?  Without that
> the regrename pass won't get a register class for the
> operand and will leave it alone.
>
Thanks! I never thought about it. It works. I will do a full testing.


Jie

[ARM] Remove register constraints from push multiple patterns (was Re: [PATCH] Provide a hook for target to disable register renaming for some instructions)

[Jie Zhang]

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

On 07/01/2010 02:03 AM, Richard Henderson wrote:
> On 06/30/2010 10:53 AM, Jie Zhang wrote:
>> When compiler an application for ARM, the GAS issued a warning:
>>
>> Warning: register range not in ascending order
>>
>> for the instruction
>>
>> push    {ip, r3, r4, lr}
>>
>> Before regrename pass, this instruction looked like
>>
>> push    {r0, r3, r4, lr}
>
> Doesn't it work just as well to simply remove the register
> constraint from the push_multi instruction?  Without that
> the regrename pass won't get a register class for the
> operand and will leave it alone.
>
According to Richard's comment, a new patch is attached.  It removes 
register constraints from all three push multiple patterns.  Since there 
are no constraints for operand 1 of *push_multi_vfp, I use a new 
predicate to make it more stricter.

Tested on arm-none-eabi for NEON and non-NEON. No regressions found.

Is it OK?

Regards,
-- 
Jie Zhang
CodeSourcery

[-- Attachment #2: gcc-arm-push-multi-remove-constraints.diff --]
[-- Type: text/x-patch, Size: 2580 bytes --]


	* config/arm/vfp.md (*push_multi_vfp): Use vfp_register_operand
	as predicate for operand 1 and remove its constraint.
	* config/arm/predicates.md (vfp_register_operand): New.
	* config/arm/arm.md (*push_multi): Remove the constraint of
	operand 1.
	(*push_fp_multi): Likewise.


Index: config/arm/vfp.md
===================================================================
--- config/arm/vfp.md	(revision 161654)
+++ config/arm/vfp.md	(working copy)
@@ -1132,7 +1132,7 @@
 (define_insn "*push_multi_vfp"
   [(match_parallel 2 "multi_register_push"
     [(set (match_operand:BLK 0 "memory_operand" "=m")
-	  (unspec:BLK [(match_operand:DF 1 "s_register_operand" "w")]
+	  (unspec:BLK [(match_operand:DF 1 "vfp_register_operand" "")]
 		      UNSPEC_PUSH_MULT))])]
   "TARGET_32BIT && TARGET_HARD_FLOAT && TARGET_VFP"
   "* return vfp_output_fstmd (operands);"
Index: config/arm/predicates.md
===================================================================
--- config/arm/predicates.md	(revision 161654)
+++ config/arm/predicates.md	(working copy)
@@ -73,6 +73,21 @@
 	      || REGNO_REG_CLASS (REGNO (op)) == FPA_REGS));
 })
 
+(define_predicate "vfp_register_operand"
+  (match_code "reg,subreg")
+{
+  if (GET_CODE (op) == SUBREG)
+    op = SUBREG_REG (op);
+
+  /* We don't consider registers whose class is NO_REGS
+     to be a register operand.  */
+  return (GET_CODE (op) == REG
+	  && (REGNO (op) >= FIRST_PSEUDO_REGISTER
+	      || REGNO_REG_CLASS (REGNO (op)) == VFP_LO_REGS
+	      || (TARGET_VFPD32
+		  && REGNO_REG_CLASS (REGNO (op)) == VFP_REGS)));
+})
+
 (define_special_predicate "subreg_lowpart_operator"
   (and (match_code "subreg")
        (match_test "subreg_lowpart_p (op)")))
Index: config/arm/arm.md
===================================================================
--- config/arm/arm.md	(revision 161654)
+++ config/arm/arm.md	(working copy)
@@ -10871,7 +10871,7 @@
 (define_insn "*push_multi"
   [(match_parallel 2 "multi_register_push"
     [(set (match_operand:BLK 0 "memory_operand" "=m")
-	  (unspec:BLK [(match_operand:SI 1 "s_register_operand" "r")]
+	  (unspec:BLK [(match_operand:SI 1 "s_register_operand" "")]
 		      UNSPEC_PUSH_MULT))])]
   "TARGET_32BIT"
   "*
@@ -10924,7 +10924,7 @@
 (define_insn "*push_fp_multi"
   [(match_parallel 2 "multi_register_push"
     [(set (match_operand:BLK 0 "memory_operand" "=m")
-	  (unspec:BLK [(match_operand:XF 1 "f_register_operand" "f")]
+	  (unspec:BLK [(match_operand:XF 1 "f_register_operand" "")]
 		      UNSPEC_PUSH_MULT))])]
   "TARGET_32BIT && TARGET_HARD_FLOAT && TARGET_FPA"
   "*

Re: [ARM] Remove register constraints from push multiple patterns (was Re: [PATCH] Provide a hook for target to disable register renaming for some instructions)

[Richard Earnshaw]

On Thu, 2010-07-01 at 21:19 +0800, Jie Zhang wrote:
> On 07/01/2010 02:03 AM, Richard Henderson wrote:
> > On 06/30/2010 10:53 AM, Jie Zhang wrote:
> >> When compiler an application for ARM, the GAS issued a warning:
> >>
> >> Warning: register range not in ascending order
> >>
> >> for the instruction
> >>
> >> push    {ip, r3, r4, lr}
> >>
> >> Before regrename pass, this instruction looked like
> >>
> >> push    {r0, r3, r4, lr}
> >
> > Doesn't it work just as well to simply remove the register
> > constraint from the push_multi instruction?  Without that
> > the regrename pass won't get a register class for the
> > operand and will leave it alone.
> >
> According to Richard's comment, a new patch is attached.  It removes 
> register constraints from all three push multiple patterns.  Since there 
> are no constraints for operand 1 of *push_multi_vfp, I use a new 
> predicate to make it more stricter.
> 
> Tested on arm-none-eabi for NEON and non-NEON. No regressions found.
> 
> Is it OK?
> 
> Regards,

OK

R.

Re: [ARM] Remove register constraints from push multiple patterns (was Re: [PATCH] Provide a hook for target to disable register renaming for some instructions)

[Jie Zhang]

On 07/01/2010 09:37 PM, Richard Earnshaw wrote:
>
> On Thu, 2010-07-01 at 21:19 +0800, Jie Zhang wrote:
>> On 07/01/2010 02:03 AM, Richard Henderson wrote:
>>> On 06/30/2010 10:53 AM, Jie Zhang wrote:
>>>> When compiler an application for ARM, the GAS issued a warning:
>>>>
>>>> Warning: register range not in ascending order
>>>>
>>>> for the instruction
>>>>
>>>> push    {ip, r3, r4, lr}
>>>>
>>>> Before regrename pass, this instruction looked like
>>>>
>>>> push    {r0, r3, r4, lr}
>>>
>>> Doesn't it work just as well to simply remove the register
>>> constraint from the push_multi instruction?  Without that
>>> the regrename pass won't get a register class for the
>>> operand and will leave it alone.
>>>
>> According to Richard's comment, a new patch is attached.  It removes
>> register constraints from all three push multiple patterns.  Since there
>> are no constraints for operand 1 of *push_multi_vfp, I use a new
>> predicate to make it more stricter.
>>
>> Tested on arm-none-eabi for NEON and non-NEON. No regressions found.
>>
>> Is it OK?
>>
>> Regards,
>
> OK
>
Thanks. Committed on trunk. OK for 4.5 branch, too?


-- 
Jie Zhang
CodeSourcery

Re: [ARM] Remove register constraints from push multiple patterns (was Re: [PATCH] Provide a hook for target to disable register renaming for some instructions)

[Richard Earnshaw]

On Sun, 2010-07-04 at 00:44 +0800, Jie Zhang wrote:
> On 07/01/2010 09:37 PM, Richard Earnshaw wrote:
> >
> > On Thu, 2010-07-01 at 21:19 +0800, Jie Zhang wrote:
> >> On 07/01/2010 02:03 AM, Richard Henderson wrote:
> >>> On 06/30/2010 10:53 AM, Jie Zhang wrote:
> >>>> When compiler an application for ARM, the GAS issued a warning:
> >>>>
> >>>> Warning: register range not in ascending order
> >>>>
> >>>> for the instruction
> >>>>
> >>>> push    {ip, r3, r4, lr}
> >>>>
> >>>> Before regrename pass, this instruction looked like
> >>>>
> >>>> push    {r0, r3, r4, lr}
> >>>
> >>> Doesn't it work just as well to simply remove the register
> >>> constraint from the push_multi instruction?  Without that
> >>> the regrename pass won't get a register class for the
> >>> operand and will leave it alone.
> >>>
> >> According to Richard's comment, a new patch is attached.  It removes
> >> register constraints from all three push multiple patterns.  Since there
> >> are no constraints for operand 1 of *push_multi_vfp, I use a new
> >> predicate to make it more stricter.
> >>
> >> Tested on arm-none-eabi for NEON and non-NEON. No regressions found.
> >>
> >> Is it OK?
> >>
> >> Regards,
> >
> > OK
> >
> Thanks. Committed on trunk. OK for 4.5 branch, too?
> 

Yes.

R.

Re: [ARM] Remove register constraints from push multiple patterns (was Re: [PATCH] Provide a hook for target to disable register renaming for some instructions)

[Jie Zhang]

On 07/05/2010 04:51 PM, Richard Earnshaw wrote:
>
> On Sun, 2010-07-04 at 00:44 +0800, Jie Zhang wrote:
>> On 07/01/2010 09:37 PM, Richard Earnshaw wrote:
>>>
>>> On Thu, 2010-07-01 at 21:19 +0800, Jie Zhang wrote:
>>>> On 07/01/2010 02:03 AM, Richard Henderson wrote:
>>>>> On 06/30/2010 10:53 AM, Jie Zhang wrote:
>>>>>> When compiler an application for ARM, the GAS issued a warning:
>>>>>>
>>>>>> Warning: register range not in ascending order
>>>>>>
>>>>>> for the instruction
>>>>>>
>>>>>> push    {ip, r3, r4, lr}
>>>>>>
>>>>>> Before regrename pass, this instruction looked like
>>>>>>
>>>>>> push    {r0, r3, r4, lr}
>>>>>
>>>>> Doesn't it work just as well to simply remove the register
>>>>> constraint from the push_multi instruction?  Without that
>>>>> the regrename pass won't get a register class for the
>>>>> operand and will leave it alone.
>>>>>
>>>> According to Richard's comment, a new patch is attached.  It removes
>>>> register constraints from all three push multiple patterns.  Since there
>>>> are no constraints for operand 1 of *push_multi_vfp, I use a new
>>>> predicate to make it more stricter.
>>>>
>>>> Tested on arm-none-eabi for NEON and non-NEON. No regressions found.
>>>>
>>>> Is it OK?
>>>>
>>>> Regards,
>>>
>>> OK
>>>
>> Thanks. Committed on trunk. OK for 4.5 branch, too?
>>
>
> Yes.
>
Thanks. Committed on 4.5 branch now.


-- 
Jie Zhang
CodeSourcery