[PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns, 2/3

[PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns, 2/3

[Carrot Wei]

Hi

This is the second part of the fixing for

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47855

This patch contains the length computation for insn patterns "*arm_movqi_insn"
and "*arm_addsi3". Since the alternatives and encodings are much more complex,
the attribute length is computed in separate C functions.

The patch has been tested on qemu with both ARM and thumb mode.

thanks
Carrot

ChangeLog:
2011-04-08  Wei Guozhi  <carrot@google.com>

        PR target/47855
        * config/arm/arm-protos.h (arm_attr_length_movqi): New prototype.
        (arm_attr_length_addsi3): Likewise.
        * config/arm/arm.c (arm_attr_length_movqi): New function.
        (arm_attr_length_addsi3): Likewise.
        * config/arm/arm.md (*arm_movqi_insn): Compute attr "length".
        (*arm_addsi3): Change "length" computation by calling C function.


Index: arm.c
===================================================================
--- arm.c	(revision 172017)
+++ arm.c	(working copy)
@@ -23694,4 +23694,179 @@ arm_preferred_rename_class (reg_class_t
     return NO_REGS;
 }

+/* Compute the atrribute "length" of insn "*arm_movqi_insn".
+   So this function MUST be kept in sync with that insn pattern.  */
+int
+arm_attr_length_movqi (rtx dst, rtx src)
+{
+  enum rtx_code dst_code = GET_CODE (dst);
+  enum rtx_code src_code = GET_CODE (src);
+  rtx address, reg_op, base;
+
+  /* ARM mode.  */
+  if (TARGET_ARM)
+    return 4;
+
+  /* Thumb2 mode.  */
+  /* Register move.  */
+  if ((dst_code == REG) && (src_code == REG))
+    return 2;
+
+  /* Load an immediate.  */
+  if ((dst_code == REG) && (src_code == CONST_INT))
+    {
+      HOST_WIDE_INT i = INTVAL (src);
+      if ((arm_regno_class (REGNO (dst)) == LO_REGS) && (i >= 0) && (i <= 255))
+	return 2;
+      else
+	return 4;
+    }
+
+  /* Load/store a byte from/to memory.  */
+  if ((dst_code == REG) && (src_code == MEM))
+    {
+      reg_op = dst;
+      address = XEXP (src, 0);
+    }
+  else
+    {
+      gcc_assert ((dst_code == MEM) && (src_code == REG));
+      reg_op = src;
+      address = XEXP (dst, 0);
+    }
+
+  if (GET_CODE (address) == PLUS)
+    {
+      rtx op1 = XEXP (address, 1);
+      base = XEXP (address, 0);
+      if (GET_CODE (op1) == REG)
+	{
+	  if (arm_regno_class (REGNO (op1)) != LO_REGS)
+	    return 4;
+	}
+      else if ((GET_CODE (op1) != CONST_INT)
+	       || (INTVAL (op1) < 0) || (INTVAL (op1) > 31))
+	return 4;
+    }
+  else
+    base = address;
+
+  if (GET_CODE (base) != REG)
+    return 4;
+  if ((arm_regno_class (REGNO (base)) == LO_REGS)
+      && (arm_regno_class (REGNO (reg_op)) == LO_REGS))
+    return 2;
+
+  return 4;
+}
+
+/* Compute the atrribute "length" of insn "*arm_addsi3".
+   So this function MUST be kept in sync with that insn pattern.  */
+int
+arm_attr_length_addsi3 (rtx dst, rtx src1, rtx src2)
+{
+  HOST_WIDE_INT v;
+
+  /* The splite case.  */
+  if (which_alternative == 5)
+    return 16;
+
+  /* ARM mode.  */
+  if (TARGET_ARM)
+    return 4;
+
+  /* Thumb2 mode.  */
+  if (which_alternative == 4)
+    {
+      /* SUB (SP minus immediate).  */
+      v = -INTVAL (src2);
+      gcc_assert (v >= 0);
+      if (((v & 3) == 0) && (v < 512))
+	return 2;
+      return 4;
+    }
+  else if (which_alternative == 3)
+    {
+      /* SUB immediate.  */
+      HOST_WIDE_INT max_offset = 7;
+      v = -INTVAL (src2);
+      gcc_assert (v >= 0);
+      if (REGNO (dst) == REGNO (src1))
+	max_offset = 255;
+      if ((arm_regno_class (REGNO (dst)) == LO_REGS)
+	  && (arm_regno_class (REGNO (src1)) == LO_REGS)
+	  && (v <= max_offset))
+	return 2;
+      return 4;
+    }
+  else if (which_alternative == 1)
+    {
+      if (REG_P (src2))                    /* ADD SP,SP,<Rm>*/
+	return 2;
+      v = INTVAL (src2);
+      if (((v & 3) == 0) && (v < 1024))    /* ADD SP,SP,#<imm>  */
+	return 2;
+      return 4;
+    }
+  else
+    {
+      if (which_alternative == 2)
+	{
+	  rtx tmp = src1; src1 = src2; src2 = tmp;
+	}
+      /* Now we are in the most general case, (which_alternative == 0).  */
+      gcc_assert (REG_P (src1));
+      if (REGNO (src1) == SP_REGNUM)
+	{
+	  if (REG_P (src2))
+	    {
+	      /* ADD <Rdm>, SP, <Rdm>  */
+	      if (REGNO (src2) == REGNO (src1))
+		return 2;
+	      return 4;
+	    }
+	  else
+	    {
+	      gcc_assert (GET_CODE (src2) == CONST_INT);
+	      v = INTVAL (src2);
+	      /* ADD <Rd>, SP, #<imm>  */
+	      if ((arm_regno_class (REGNO (dst)) == LO_REGS)
+		  && ((v & 3) == 0) && (v < 1024))
+		return 2;
+	      return 4;
+	    }
+	}
+      else
+	{
+	  if (REG_P (src2))
+	    {
+	      /* ADD <Rdn>, <Rm>  */
+	      if ((REGNO (dst) == REGNO (src1))
+		  || (REGNO (dst) == REGNO (src2)))
+		return 2;
+	      /* ADDS <Rd>, <Rn>, <Rm>  */
+	      else if ((arm_regno_class (REGNO (dst)) == LO_REGS)
+		       && (arm_regno_class (REGNO (src1)) == LO_REGS)
+		       && (arm_regno_class (REGNO (src2)) == LO_REGS))
+		return 2;
+	      return 4;
+	    }
+	  else
+	    {
+	      /* ADD immediate.  */
+	      HOST_WIDE_INT max_offset = 7;
+	      gcc_assert (GET_CODE (src2) == CONST_INT);
+	      v = INTVAL (src2);
+	      if (REGNO (dst) == REGNO (src1))
+		max_offset = 255;
+	      if ((arm_regno_class (REGNO (dst)) == LO_REGS)
+		  && (arm_regno_class (REGNO (src1)) == LO_REGS)
+		  && (v <= max_offset))
+		return 2;
+	      return 4;
+	    }
+	}
+    }
+}
+
 #include "gt-arm.h"
Index: arm-protos.h
===================================================================
--- arm-protos.h	(revision 172017)
+++ arm-protos.h	(working copy)
@@ -152,6 +152,8 @@ extern void arm_expand_sync (enum machin
 extern const char *arm_output_memory_barrier (rtx *);
 extern const char *arm_output_sync_insn (rtx, rtx *);
 extern unsigned int arm_sync_loop_insns (rtx , rtx *);
+extern int arm_attr_length_movqi (rtx, rtx);
+extern int arm_attr_length_addsi3 (rtx, rtx, rtx);

 #if defined TREE_CODE
 extern void arm_init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx, tree);
Index: arm.md
===================================================================
--- arm.md	(revision 172017)
+++ arm.md	(working copy)
@@ -730,8 +730,10 @@
 		      operands[1], 0);
   DONE;
   "
-  [(set_attr "length" "4,4,4,4,4,16")
-   (set_attr "predicable" "yes")]
+  [(set_attr "predicable" "yes")
+   (set (attr "length")
+	(symbol_ref
+	  "arm_attr_length_addsi3 (operands[0], operands[1], operands[2])"))]
 )

 (define_insn_and_split "*thumb1_addsi3"
@@ -5958,7 +5960,9 @@
    str%(b%)\\t%1, %0"
   [(set_attr "type" "*,*,load1,store1")
    (set_attr "insn" "mov,mvn,*,*")
-   (set_attr "predicable" "yes")]
+   (set_attr "predicable" "yes")
+   (set (attr "length")
+	(symbol_ref "arm_attr_length_movqi (operands[0], operands[1])"))]
 )

 (define_insn "*thumb1_movqi_insn"

Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns, 2/3

[Ramana Radhakrishnan]

On 08/04/11 10:57, Carrot Wei wrote:
> Hi
>
> This is the second part of the fixing for
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47855
>
> This patch contains the length computation for insn patterns "*arm_movqi_insn"
> and "*arm_addsi3". Since the alternatives and encodings are much more complex,
> the attribute length is computed in separate C functions.


Sorry, no. This is potentially a maintenance pain. It hardcodes 
alternatives from a pattern elsewhere in the C file. I don't like doing 
this unless we have to with the sync primitives or with push_multi. In 
this case I'm not convinced we need such functions in the .c file.

Why can't we use the "enabled" attribute here with appropriate 
constraints for everything other than the memory cases (even there we 
might be able to invent some new constraints) ?

Also a note about programming style. There are the helper macros like 
REG_P, CONST_INT_P and MEM_P which remove the necessity for checks like

GET_CODE (x) == y where y E { REG, CONST_INT, MEM}


cheers
Ramana

>
> The patch has been tested on qemu with both ARM and thumb mode.
>
> thanks
> Carrot
>
> ChangeLog:
> 2011-04-08  Wei Guozhi<carrot@google.com>
>
>          PR target/47855
>          * config/arm/arm-protos.h (arm_attr_length_movqi): New prototype.
>          (arm_attr_length_addsi3): Likewise.
>          * config/arm/arm.c (arm_attr_length_movqi): New function.
>          (arm_attr_length_addsi3): Likewise.
>          * config/arm/arm.md (*arm_movqi_insn): Compute attr "length".
>          (*arm_addsi3): Change "length" computation by calling C function.
>
>
> Index: arm.c
> ===================================================================
> --- arm.c	(revision 172017)
> +++ arm.c	(working copy)
> @@ -23694,4 +23694,179 @@ arm_preferred_rename_class (reg_class_t
>       return NO_REGS;
>   }
>
> +/* Compute the atrribute "length" of insn "*arm_movqi_insn".
> +   So this function MUST be kept in sync with that insn pattern.  */
> +int
> +arm_attr_length_movqi (rtx dst, rtx src)
> +{
> +  enum rtx_code dst_code = GET_CODE (dst);
> +  enum rtx_code src_code = GET_CODE (src);
> +  rtx address, reg_op, base;
> +
> +  /* ARM mode.  */
> +  if (TARGET_ARM)
> +    return 4;
> +
> +  /* Thumb2 mode.  */
> +  /* Register move.  */
> +  if ((dst_code == REG)&&  (src_code == REG))
> +    return 2;
> +
> +  /* Load an immediate.  */
> +  if ((dst_code == REG)&&  (src_code == CONST_INT))
> +    {
> +      HOST_WIDE_INT i = INTVAL (src);
> +      if ((arm_regno_class (REGNO (dst)) == LO_REGS)&&  (i>= 0)&&  (i<= 255))
> +	return 2;
> +      else
> +	return 4;
> +    }
> +
> +  /* Load/store a byte from/to memory.  */
> +  if ((dst_code == REG)&&  (src_code == MEM))
> +    {
> +      reg_op = dst;
> +      address = XEXP (src, 0);
> +    }
> +  else
> +    {
> +      gcc_assert ((dst_code == MEM)&&  (src_code == REG));
> +      reg_op = src;
> +      address = XEXP (dst, 0);
> +    }
> +
> +  if (GET_CODE (address) == PLUS)
> +    {
> +      rtx op1 = XEXP (address, 1);
> +      base = XEXP (address, 0);
> +      if (GET_CODE (op1) == REG)
> +	{
> +	  if (arm_regno_class (REGNO (op1)) != LO_REGS)
> +	    return 4;
> +	}
> +      else if ((GET_CODE (op1) != CONST_INT)
> +	       || (INTVAL (op1)<  0) || (INTVAL (op1)>  31))
> +	return 4;
> +    }
> +  else
> +    base = address;
> +
> +  if (GET_CODE (base) != REG)
> +    return 4;
> +  if ((arm_regno_class (REGNO (base)) == LO_REGS)
> +&&  (arm_regno_class (REGNO (reg_op)) == LO_REGS))
> +    return 2;
> +
> +  return 4;
> +}
> +
> +/* Compute the atrribute "length" of insn "*arm_addsi3".
> +   So this function MUST be kept in sync with that insn pattern.  */
> +int
> +arm_attr_length_addsi3 (rtx dst, rtx src1, rtx src2)
> +{
> +  HOST_WIDE_INT v;
> +
> +  /* The splite case.  */
> +  if (which_alternative == 5)
> +    return 16;
> +
> +  /* ARM mode.  */
> +  if (TARGET_ARM)
> +    return 4;
> +
> +  /* Thumb2 mode.  */
> +  if (which_alternative == 4)
> +    {
> +      /* SUB (SP minus immediate).  */
> +      v = -INTVAL (src2);
> +      gcc_assert (v>= 0);
> +      if (((v&  3) == 0)&&  (v<  512))
> +	return 2;
> +      return 4;
> +    }
> +  else if (which_alternative == 3)
> +    {
> +      /* SUB immediate.  */
> +      HOST_WIDE_INT max_offset = 7;
> +      v = -INTVAL (src2);
> +      gcc_assert (v>= 0);
> +      if (REGNO (dst) == REGNO (src1))
> +	max_offset = 255;
> +      if ((arm_regno_class (REGNO (dst)) == LO_REGS)
> +	&&  (arm_regno_class (REGNO (src1)) == LO_REGS)
> +	&&  (v<= max_offset))
> +	return 2;
> +      return 4;
> +    }
> +  else if (which_alternative == 1)
> +    {
> +      if (REG_P (src2))                    /* ADD SP,SP,<Rm>*/
> +	return 2;
> +      v = INTVAL (src2);
> +      if (((v&  3) == 0)&&  (v<  1024))    /* ADD SP,SP,#<imm>   */
> +	return 2;
> +      return 4;
> +    }
> +  else
> +    {
> +      if (which_alternative == 2)
> +	{
> +	  rtx tmp = src1; src1 = src2; src2 = tmp;
> +	}
> +      /* Now we are in the most general case, (which_alternative == 0).  */
> +      gcc_assert (REG_P (src1));
> +      if (REGNO (src1) == SP_REGNUM)
> +	{
> +	  if (REG_P (src2))
> +	    {
> +	      /* ADD<Rdm>, SP,<Rdm>   */
> +	      if (REGNO (src2) == REGNO (src1))
> +		return 2;
> +	      return 4;
> +	    }
> +	  else
> +	    {
> +	      gcc_assert (GET_CODE (src2) == CONST_INT);
> +	      v = INTVAL (src2);
> +	      /* ADD<Rd>, SP, #<imm>   */
> +	      if ((arm_regno_class (REGNO (dst)) == LO_REGS)
> +		&&  ((v&  3) == 0)&&  (v<  1024))
> +		return 2;
> +	      return 4;
> +	    }
> +	}
> +      else
> +	{
> +	  if (REG_P (src2))
> +	    {
> +	      /* ADD<Rdn>,<Rm>   */
> +	      if ((REGNO (dst) == REGNO (src1))
> +		  || (REGNO (dst) == REGNO (src2)))
> +		return 2;
> +	      /* ADDS<Rd>,<Rn>,<Rm>   */
> +	      else if ((arm_regno_class (REGNO (dst)) == LO_REGS)
> +		&&  (arm_regno_class (REGNO (src1)) == LO_REGS)
> +		&&  (arm_regno_class (REGNO (src2)) == LO_REGS))
> +		return 2;
> +	      return 4;
> +	    }
> +	  else
> +	    {
> +	      /* ADD immediate.  */
> +	      HOST_WIDE_INT max_offset = 7;
> +	      gcc_assert (GET_CODE (src2) == CONST_INT);
> +	      v = INTVAL (src2);
> +	      if (REGNO (dst) == REGNO (src1))
> +		max_offset = 255;
> +	      if ((arm_regno_class (REGNO (dst)) == LO_REGS)
> +		&&  (arm_regno_class (REGNO (src1)) == LO_REGS)
> +		&&  (v<= max_offset))
> +		return 2;
> +	      return 4;
> +	    }
> +	}
> +    }
> +}
> +
>   #include "gt-arm.h"
> Index: arm-protos.h
> ===================================================================
> --- arm-protos.h	(revision 172017)
> +++ arm-protos.h	(working copy)
> @@ -152,6 +152,8 @@ extern void arm_expand_sync (enum machin
>   extern const char *arm_output_memory_barrier (rtx *);
>   extern const char *arm_output_sync_insn (rtx, rtx *);
>   extern unsigned int arm_sync_loop_insns (rtx , rtx *);
> +extern int arm_attr_length_movqi (rtx, rtx);
> +extern int arm_attr_length_addsi3 (rtx, rtx, rtx);
>
>   #if defined TREE_CODE
>   extern void arm_init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx, tree);
> Index: arm.md
> ===================================================================
> --- arm.md	(revision 172017)
> +++ arm.md	(working copy)
> @@ -730,8 +730,10 @@
>   		      operands[1], 0);
>     DONE;
>     "
> -  [(set_attr "length" "4,4,4,4,4,16")
> -   (set_attr "predicable" "yes")]
> +  [(set_attr "predicable" "yes")
> +   (set (attr "length")
> +	(symbol_ref
> +	  "arm_attr_length_addsi3 (operands[0], operands[1], operands[2])"))]
>   )
>
>   (define_insn_and_split "*thumb1_addsi3"
> @@ -5958,7 +5960,9 @@
>      str%(b%)\\t%1, %0"
>     [(set_attr "type" "*,*,load1,store1")
>      (set_attr "insn" "mov,mvn,*,*")
> -   (set_attr "predicable" "yes")]
> +   (set_attr "predicable" "yes")
> +   (set (attr "length")
> +	(symbol_ref "arm_attr_length_movqi (operands[0], operands[1])"))]
>   )
>
>   (define_insn "*thumb1_movqi_insn"

Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns, 2/3

[Carrot Wei]

On Fri, Apr 8, 2011 at 6:51 PM, Ramana Radhakrishnan
<ramana.radhakrishnan@linaro.org> wrote:
> On 08/04/11 10:57, Carrot Wei wrote:
>>
>> Hi
>>
>> This is the second part of the fixing for
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47855
>>
>> This patch contains the length computation for insn patterns
>> "*arm_movqi_insn"
>> and "*arm_addsi3". Since the alternatives and encodings are much more
>> complex,
>> the attribute length is computed in separate C functions.

> Sorry, no. This is potentially a maintenance pain. It hardcodes alternatives
> from a pattern elsewhere in the C file. I don't like doing this unless we
> have to with the sync primitives or with push_multi. In this case I'm not
> convinced we need such functions in the .c file.
>
> Why can't we use the "enabled" attribute here with appropriate constraints
> for everything other than the memory cases (even there we might be able to
> invent some new constraints) ?
>
> Also a note about programming style. There are the helper macros like REG_P,
> CONST_INT_P and MEM_P which remove the necessity for checks like
>
> GET_CODE (x) == y where y E { REG, CONST_INT, MEM}

Hi Ramana

As you suggested I created several new constraints, and use the
"enabled" attribute to split the current alternatives in this new
patch. It has been tested on arm qemu without regression.

thanks
Carrot


ChangeLog:
2011-04-14  Wei Guozhi  <carrot@google.com>

        PR target/47855
        * config/arm/arm-protos.h (thumb1_legitimate_address_p): New prototype.
        * config/arm/arm.c (thumb1_legitimate_address_p): Remove the static
        linkage.
        * config/arm/constraints.md (Pq, Pr, Pz, Uu): New constraints.
        (Pd): Also apply to thumb2.
        * config/arm/arm.md (*arm_movqi_insn): Compute attr "length".
        (*arm_addsi3): Change "length" computation by splitting alternatives.


Index: arm.c
===================================================================
--- arm.c	(revision 172353)
+++ arm.c	(working copy)
@@ -5772,7 +5772,7 @@ thumb1_index_register_rtx_p (rtx x, int
    addresses based on the frame pointer or arg pointer until the
    reload pass starts.  This is so that eliminating such addresses
    into stack based ones won't produce impossible code.  */
-static int
+int
 thumb1_legitimate_address_p (enum machine_mode mode, rtx x, int strict_p)
 {
   /* ??? Not clear if this is right.  Experiment.  */
Index: arm-protos.h
===================================================================
--- arm-protos.h	(revision 172353)
+++ arm-protos.h	(working copy)
@@ -58,6 +58,7 @@ extern bool arm_legitimize_reload_addres
 					   int);
 extern rtx thumb_legitimize_reload_address (rtx *, enum machine_mode, int, int,
 					    int);
+extern int thumb1_legitimate_address_p (enum machine_mode, rtx, int);
 extern int arm_const_double_rtx (rtx);
 extern int neg_const_double_rtx_ok_for_fpa (rtx);
 extern int vfp3_const_double_rtx (rtx);
Index: constraints.md
===================================================================
--- constraints.md	(revision 172353)
+++ constraints.md	(working copy)
@@ -30,12 +30,14 @@

 ;; The following multi-letter normal constraints have been used:
 ;; in ARM/Thumb-2 state: Da, Db, Dc, Dn, Dl, DL, Dv, Dy, Di, Dz
-;; in Thumb-1 state: Pa, Pb, Pc, Pd
-;; in Thumb-2 state: Ps, Pt, Pu, Pv, Pw, Px, Py
+;; in Thumb-1 state: Pa, Pb, Pc
+;; in Thumb-2 state: Pq, Pr, Ps, Pt, Pu, Pv, Pw, Px, Py, Pz
+;; in Thumb state: Pd

 ;; The following memory constraints have been used:
 ;; in ARM/Thumb-2 state: Q, Ut, Uv, Uy, Un, Um, Us
 ;; in ARM state: Uq
+;; in Thumb state: Uu


 (define_register_constraint "f" "TARGET_ARM ? FPA_REGS : NO_REGS"
@@ -155,9 +157,23 @@
   		    && ival > 1020 && ival <= 1275")))

 (define_constraint "Pd"
-  "@internal In Thumb-1 state a constant in the range 0 to 7"
+  "@internal In Thumb state a constant in the range 0 to 7"
   (and (match_code "const_int")
-       (match_test "TARGET_THUMB1 && ival >= 0 && ival <= 7")))
+       (match_test "TARGET_THUMB && ival >= 0 && ival <= 7")))
+
+(define_constraint "Pq"
+  "@internal In Thumb-2 state a constant that is a multiple of 4 in the
+   range 0-1020"
+  (and (match_code "const_int")
+       (match_test "TARGET_THUMB2
+		    && ival >= 0 && ival <= 508 && (ival & 3) == 0")))
+
+(define_constraint "Pr"
+  "@internal In Thumb-2 state a constant that is a multiple of 4 in the
+   range 0-508"
+  (and (match_code "const_int")
+       (match_test "TARGET_THUMB2
+		    && ival >= 0 && ival <= 508 && (ival & 3) == 0")))

 (define_constraint "Ps"
   "@internal In Thumb-2 state a constant in the range -255 to +255"
@@ -194,6 +210,13 @@
   (and (match_code "const_int")
        (match_test "TARGET_THUMB2 && ival >= 0 && ival <= 255")))

+(define_constraint "Pz"
+  "@internal In Thumb-2 state a constant that is a multiple of 4 in
+   the range -508 to 0"
+  (and (match_code "const_int")
+       (match_test "TARGET_THUMB2 && ival >= -508 && ival <= 0
+		    && (ival & 3) == 0")))
+
 (define_constraint "G"
  "In ARM/Thumb-2 state a valid FPA immediate constant."
  (and (match_code "const_double")
@@ -332,6 +355,14 @@
  (and (match_code "mem")
       (match_test "REG_P (XEXP (op, 0))")))

+(define_memory_constraint "Uu"
+ "@internal
+  In Thumb state an address that is valid in 16bit encoding."
+ (and (match_code "mem")
+      (match_test "TARGET_THUMB
+		   && thumb1_legitimate_address_p (GET_MODE (op), XEXP (op, 0),
+						   0)")))
+
 ;; We used to have constraint letters for S and R in ARM state, but
 ;; all uses of these now appear to have been removed.

Index: arm.md
===================================================================
--- arm.md	(revision 172353)
+++ arm.md	(working copy)
@@ -707,16 +707,27 @@
 ;;  (plus (reg rN) (reg sp)) into (reg rN).  In this case reload will
 ;; put the duplicated register first, and not try the commutative version.
 (define_insn_and_split "*arm_addsi3"
-  [(set (match_operand:SI          0 "s_register_operand" "=r, k,r,r, k,r")
-	(plus:SI (match_operand:SI 1 "s_register_operand" "%rk,k,r,rk,k,rk")
-		 (match_operand:SI 2 "reg_or_int_operand" "rI,rI,k,L, L,?n")))]
+  [(set (match_operand:SI          0 "s_register_operand"
"=r,l,r,l,l,l,r, k,k,r,r,l,l,r, k,k,r")
+	(plus:SI (match_operand:SI 1 "s_register_operand"
"%k,k,0,l,0,l,rk,k,k,0,r,0,l,rk,k,k,rk")
+		 (match_operand:SI 2 "reg_or_int_operand"
"0,Pq,r,l,Py,Pd,rI,rPr,rI,k,k,Pv,Px,L,Pz,L,?n")))]
   "TARGET_32BIT"
   "@
    add%?\\t%0, %1, %2
    add%?\\t%0, %1, %2
+   add%?\\t%0, %1, %2
+   add%?\\t%0, %1, %2
+   add%?\\t%0, %1, %2
+   add%?\\t%0, %1, %2
+   add%?\\t%0, %1, %2
+   add%?\\t%0, %1, %2
+   add%?\\t%0, %1, %2
+   add%?\\t%0, %2, %1
    add%?\\t%0, %2, %1
    sub%?\\t%0, %1, #%n2
    sub%?\\t%0, %1, #%n2
+   sub%?\\t%0, %1, #%n2
+   sub%?\\t%0, %1, #%n2
+   sub%?\\t%0, %1, #%n2
    #"
   "TARGET_32BIT
    && GET_CODE (operands[2]) == CONST_INT
@@ -730,8 +741,9 @@
 		      operands[1], 0);
   DONE;
   "
-  [(set_attr "length" "4,4,4,4,4,16")
-   (set_attr "predicable" "yes")]
+  [(set_attr "length" "2,2,2,2,2,2,4,2,4,2,4,2,2,4,2,4,16")
+   (set_attr "predicable" "yes")
+   (set_attr "arch"
"t2,t2,t2,t2,t2,t2,any,t2,any,t2,any,t2,t2,any,t2,any,any")]
 )

 (define_insn_and_split "*thumb1_addsi3"
@@ -5946,19 +5958,25 @@


 (define_insn "*arm_movqi_insn"
-  [(set (match_operand:QI 0 "nonimmediate_operand" "=r,r,r,m")
-	(match_operand:QI 1 "general_operand" "rI,K,m,r"))]
+  [(set (match_operand:QI 0 "nonimmediate_operand" "=r,l,r,r,l,Uu,r,m")
+	(match_operand:QI 1 "general_operand" "r,Py,rI,K,Uu,l,m,r"))]
   "TARGET_32BIT
    && (   register_operand (operands[0], QImode)
        || register_operand (operands[1], QImode))"
   "@
    mov%?\\t%0, %1
+   mov%?\\t%0, %1
+   mov%?\\t%0, %1
    mvn%?\\t%0, #%B1
    ldr%(b%)\\t%0, %1
+   str%(b%)\\t%1, %0
+   ldr%(b%)\\t%0, %1
    str%(b%)\\t%1, %0"
-  [(set_attr "type" "*,*,load1,store1")
-   (set_attr "insn" "mov,mvn,*,*")
-   (set_attr "predicable" "yes")]
+  [(set_attr "type" "*,*,*,*,load1,store1,load1,store1")
+   (set_attr "insn" "mov,mov,mov,mvn,*,*,*,*")
+   (set_attr "predicable" "yes")
+   (set_attr "arch" "t2,t2,any,any,t2,t2,any,any")
+   (set_attr "length" "2,2,4,4,2,2,4,4")]
 )

 (define_insn "*thumb1_movqi_insn"

Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns, 2/3

[Richard Earnshaw]

On Thu, 2011-04-14 at 21:19 +0800, Carrot Wei wrote:
> On Fri, Apr 8, 2011 at 6:51 PM, Ramana Radhakrishnan
> <ramana.radhakrishnan@linaro.org> wrote:
> > On 08/04/11 10:57, Carrot Wei wrote:
> >>
> >> Hi
> >>
> >> This is the second part of the fixing for
> >>
> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47855
> >>
> >> This patch contains the length computation for insn patterns
> >> "*arm_movqi_insn"
> >> and "*arm_addsi3". Since the alternatives and encodings are much more
> >> complex,
> >> the attribute length is computed in separate C functions.
> 
> > Sorry, no. This is potentially a maintenance pain. It hardcodes alternatives
> > from a pattern elsewhere in the C file. I don't like doing this unless we
> > have to with the sync primitives or with push_multi. In this case I'm not
> > convinced we need such functions in the .c file.
> >
> > Why can't we use the "enabled" attribute here with appropriate constraints
> > for everything other than the memory cases (even there we might be able to
> > invent some new constraints) ?
> >
> > Also a note about programming style. There are the helper macros like REG_P,
> > CONST_INT_P and MEM_P which remove the necessity for checks like
> >
> > GET_CODE (x) == y where y E { REG, CONST_INT, MEM}
> 
> Hi Ramana
> 
> As you suggested I created several new constraints, and use the
> "enabled" attribute to split the current alternatives in this new
> patch. It has been tested on arm qemu without regression.
> 
> thanks
> Carrot


Sorry, I don't think this approach can work.  Certainly not with the way
the compiler currently works, and especially for mov and add insns. 

These instructions are only 2 bytes long if either:
1) They clobber the condition code register or
2) They occur inside an IT block.

We can't tell either of these from the pattern, so you're
underestimating the length of the instruction in some circumstances by
claiming that they are only 2 bytes long.  That /will/ lead to broken
code someday.

We can't add potential clobbers to mov and add patterns because that
will break reload which relies on these patterns being simple-set insns
with no added baggage.  It *might* be possible to add clobbers to other
operations, but that will then most-likely upset instruction scheduling
(I think the scheduler treats two insns that clobber the same hard reg
as being strongly ordered).  Putting in the clobber too early will
certainly affect cond-exec generation.

In short, I'm not aware of a simple way to address this problem so that
we get accurate length information, but minimal impact on other passes
in the compiler.

R.

Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns, 2/3

[Carrot Wei]

Hi Richard

Thank you for the detailed explanation. It sounds like an inherent
difficulty of rtl passes. Then the only opportunity is ldrb/strb
instructions because they never affect cc registers.

thanks
Carrot

On Fri, Apr 15, 2011 at 9:34 PM, Richard Earnshaw <rearnsha@arm.com> wrote:
>
> On Thu, 2011-04-14 at 21:19 +0800, Carrot Wei wrote:
>> On Fri, Apr 8, 2011 at 6:51 PM, Ramana Radhakrishnan
>> <ramana.radhakrishnan@linaro.org> wrote:
>> > On 08/04/11 10:57, Carrot Wei wrote:
>> >>
>> >> Hi
>> >>
>> >> This is the second part of the fixing for
>> >>
>> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47855
>> >>
>> >> This patch contains the length computation for insn patterns
>> >> "*arm_movqi_insn"
>> >> and "*arm_addsi3". Since the alternatives and encodings are much more
>> >> complex,
>> >> the attribute length is computed in separate C functions.
>>
>> > Sorry, no. This is potentially a maintenance pain. It hardcodes alternatives
>> > from a pattern elsewhere in the C file. I don't like doing this unless we
>> > have to with the sync primitives or with push_multi. In this case I'm not
>> > convinced we need such functions in the .c file.
>> >
>> > Why can't we use the "enabled" attribute here with appropriate constraints
>> > for everything other than the memory cases (even there we might be able to
>> > invent some new constraints) ?
>> >
>> > Also a note about programming style. There are the helper macros like REG_P,
>> > CONST_INT_P and MEM_P which remove the necessity for checks like
>> >
>> > GET_CODE (x) == y where y E { REG, CONST_INT, MEM}
>>
>> Hi Ramana
>>
>> As you suggested I created several new constraints, and use the
>> "enabled" attribute to split the current alternatives in this new
>> patch. It has been tested on arm qemu without regression.
>>
>> thanks
>> Carrot
>
>
> Sorry, I don't think this approach can work.  Certainly not with the way
> the compiler currently works, and especially for mov and add insns.
>
> These instructions are only 2 bytes long if either:
> 1) They clobber the condition code register or
> 2) They occur inside an IT block.
>
> We can't tell either of these from the pattern, so you're
> underestimating the length of the instruction in some circumstances by
> claiming that they are only 2 bytes long.  That /will/ lead to broken
> code someday.
>
> We can't add potential clobbers to mov and add patterns because that
> will break reload which relies on these patterns being simple-set insns
> with no added baggage.  It *might* be possible to add clobbers to other
> operations, but that will then most-likely upset instruction scheduling
> (I think the scheduler treats two insns that clobber the same hard reg
> as being strongly ordered).  Putting in the clobber too early will
> certainly affect cond-exec generation.
>
> In short, I'm not aware of a simple way to address this problem so that
> we get accurate length information, but minimal impact on other passes
> in the compiler.
>
> R.
>
>
>

Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns, 2/3

[Richard Earnshaw]

On Sat, 2011-04-16 at 12:34 +0800, Carrot Wei wrote:
> Hi Richard
> 
> Thank you for the detailed explanation. It sounds like an inherent
> difficulty of rtl passes. Then the only opportunity is ldrb/strb
> instructions because they never affect cc registers.

There are also some comparison operations that are also known to be 2
bytes (because they are known to set the condition codes).  But yes, the
scope here is quite limited.

R.

> 
> thanks
> Carrot
> 
> On Fri, Apr 15, 2011 at 9:34 PM, Richard Earnshaw <rearnsha@arm.com> wrote:
> >
> > On Thu, 2011-04-14 at 21:19 +0800, Carrot Wei wrote:
> >> On Fri, Apr 8, 2011 at 6:51 PM, Ramana Radhakrishnan
> >> <ramana.radhakrishnan@linaro.org> wrote:
> >> > On 08/04/11 10:57, Carrot Wei wrote:
> >> >>
> >> >> Hi
> >> >>
> >> >> This is the second part of the fixing for
> >> >>
> >> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47855
> >> >>
> >> >> This patch contains the length computation for insn patterns
> >> >> "*arm_movqi_insn"
> >> >> and "*arm_addsi3". Since the alternatives and encodings are much more
> >> >> complex,
> >> >> the attribute length is computed in separate C functions.
> >>
> >> > Sorry, no. This is potentially a maintenance pain. It hardcodes alternatives
> >> > from a pattern elsewhere in the C file. I don't like doing this unless we
> >> > have to with the sync primitives or with push_multi. In this case I'm not
> >> > convinced we need such functions in the .c file.
> >> >
> >> > Why can't we use the "enabled" attribute here with appropriate constraints
> >> > for everything other than the memory cases (even there we might be able to
> >> > invent some new constraints) ?
> >> >
> >> > Also a note about programming style. There are the helper macros like REG_P,
> >> > CONST_INT_P and MEM_P which remove the necessity for checks like
> >> >
> >> > GET_CODE (x) == y where y E { REG, CONST_INT, MEM}
> >>
> >> Hi Ramana
> >>
> >> As you suggested I created several new constraints, and use the
> >> "enabled" attribute to split the current alternatives in this new
> >> patch. It has been tested on arm qemu without regression.
> >>
> >> thanks
> >> Carrot
> >
> >
> > Sorry, I don't think this approach can work.  Certainly not with the way
> > the compiler currently works, and especially for mov and add insns.
> >
> > These instructions are only 2 bytes long if either:
> > 1) They clobber the condition code register or
> > 2) They occur inside an IT block.
> >
> > We can't tell either of these from the pattern, so you're
> > underestimating the length of the instruction in some circumstances by
> > claiming that they are only 2 bytes long.  That /will/ lead to broken
> > code someday.
> >
> > We can't add potential clobbers to mov and add patterns because that
> > will break reload which relies on these patterns being simple-set insns
> > with no added baggage.  It *might* be possible to add clobbers to other
> > operations, but that will then most-likely upset instruction scheduling
> > (I think the scheduler treats two insns that clobber the same hard reg
> > as being strongly ordered).  Putting in the clobber too early will
> > certainly affect cond-exec generation.
> >
> > In short, I'm not aware of a simple way to address this problem so that
> > we get accurate length information, but minimal impact on other passes
> > in the compiler.
> >
> > R.
> >
> >
> >
> 

Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns, 2/3

[Carrot Wei]

On Mon, Apr 18, 2011 at 9:33 PM, Richard Earnshaw <rearnsha@arm.com> wrote:
>
> On Sat, 2011-04-16 at 12:34 +0800, Carrot Wei wrote:
>> Hi Richard
>>
>> Thank you for the detailed explanation. It sounds like an inherent
>> difficulty of rtl passes. Then the only opportunity is ldrb/strb
>> instructions because they never affect cc registers.
>
> There are also some comparison operations that are also known to be 2
> bytes (because they are known to set the condition codes).  But yes, the
> scope here is quite limited.
>
> R.

So now this version only computes the correct length of ldrd/strb in insn
arm_movqi_insn. Tested on arm qemu without regression.

thanks
Carrot


ChangeLog:
2011-04-18  Wei Guozhi  <carrot@google.com>

        PR target/47855
        * config/arm/arm-protos.h (thumb1_legitimate_address_p): New prototype.
        * config/arm/arm.c (thumb1_legitimate_address_p): Remove the static
        linkage.
        * config/arm/constraints.md (Uu): New constraint.
        * config/arm/arm.md (*arm_movqi_insn): Compute attr "length".


Index: arm.c
===================================================================
--- arm.c	(revision 172353)
+++ arm.c	(working copy)
@@ -5772,7 +5772,7 @@ thumb1_index_register_rtx_p (rtx x, int
    addresses based on the frame pointer or arg pointer until the
    reload pass starts.  This is so that eliminating such addresses
    into stack based ones won't produce impossible code.  */
-static int
+int
 thumb1_legitimate_address_p (enum machine_mode mode, rtx x, int strict_p)
 {
   /* ??? Not clear if this is right.  Experiment.  */
Index: arm-protos.h
===================================================================
--- arm-protos.h	(revision 172353)
+++ arm-protos.h	(working copy)
@@ -58,6 +58,7 @@ extern bool arm_legitimize_reload_addres
 					   int);
 extern rtx thumb_legitimize_reload_address (rtx *, enum machine_mode, int, int,
 					    int);
+extern int thumb1_legitimate_address_p (enum machine_mode, rtx, int);
 extern int arm_const_double_rtx (rtx);
 extern int neg_const_double_rtx_ok_for_fpa (rtx);
 extern int vfp3_const_double_rtx (rtx);
Index: constraints.md
===================================================================
--- constraints.md	(revision 172353)
+++ constraints.md	(working copy)
@@ -36,6 +36,7 @@
 ;; The following memory constraints have been used:
 ;; in ARM/Thumb-2 state: Q, Ut, Uv, Uy, Un, Um, Us
 ;; in ARM state: Uq
+;; in Thumb state: Uu


 (define_register_constraint "f" "TARGET_ARM ? FPA_REGS : NO_REGS"
@@ -332,6 +333,14 @@
  (and (match_code "mem")
       (match_test "REG_P (XEXP (op, 0))")))

+(define_memory_constraint "Uu"
+ "@internal
+  In Thumb state an address that is valid in 16bit encoding."
+ (and (match_code "mem")
+      (match_test "TARGET_THUMB
+		   && thumb1_legitimate_address_p (GET_MODE (op), XEXP (op, 0),
+						   0)")))
+
 ;; We used to have constraint letters for S and R in ARM state, but
 ;; all uses of these now appear to have been removed.

Index: arm.md
===================================================================
--- arm.md	(revision 172353)
+++ arm.md	(working copy)
@@ -5946,8 +5946,8 @@


 (define_insn "*arm_movqi_insn"
-  [(set (match_operand:QI 0 "nonimmediate_operand" "=r,r,r,m")
-	(match_operand:QI 1 "general_operand" "rI,K,m,r"))]
+  [(set (match_operand:QI 0 "nonimmediate_operand" "=r,r,l,Uu,r,m")
+	(match_operand:QI 1 "general_operand" "rI,K,Uu,l,m,r"))]
   "TARGET_32BIT
    && (   register_operand (operands[0], QImode)
        || register_operand (operands[1], QImode))"
@@ -5955,10 +5955,14 @@
    mov%?\\t%0, %1
    mvn%?\\t%0, #%B1
    ldr%(b%)\\t%0, %1
+   str%(b%)\\t%1, %0
+   ldr%(b%)\\t%0, %1
    str%(b%)\\t%1, %0"
-  [(set_attr "type" "*,*,load1,store1")
-   (set_attr "insn" "mov,mvn,*,*")
-   (set_attr "predicable" "yes")]
+  [(set_attr "type" "*,*,load1,store1,load1,store1")
+   (set_attr "insn" "mov,mvn,*,*,*,*")
+   (set_attr "predicable" "yes")
+   (set_attr "arch" "any,any,t2,t2,any,any")
+   (set_attr "length" "4,4,2,2,4,4")]
 )

 (define_insn "*thumb1_movqi_insn"

Re: [PATCH, ARM] PR47855 Compute attr "length" for some thumb2 insns, 2/3

[Richard Earnshaw]

On Mon, 2011-04-18 at 22:17 +0800, Carrot Wei wrote:
> On Mon, Apr 18, 2011 at 9:33 PM, Richard Earnshaw <rearnsha@arm.com> wrote:
> >
> > On Sat, 2011-04-16 at 12:34 +0800, Carrot Wei wrote:
> >> Hi Richard
> >>
> >> Thank you for the detailed explanation. It sounds like an inherent
> >> difficulty of rtl passes. Then the only opportunity is ldrb/strb
> >> instructions because they never affect cc registers.
> >
> > There are also some comparison operations that are also known to be 2
> > bytes (because they are known to set the condition codes).  But yes, the
> > scope here is quite limited.
> >
> > R.
> 
> So now this version only computes the correct length of ldrd/strb in insn
> arm_movqi_insn. Tested on arm qemu without regression.
> 
> thanks
> Carrot
> 
> 
> ChangeLog:
> 2011-04-18  Wei Guozhi  <carrot@google.com>
> 
>         PR target/47855
>         * config/arm/arm-protos.h (thumb1_legitimate_address_p): New prototype.
>         * config/arm/arm.c (thumb1_legitimate_address_p): Remove the static
>         linkage.
>         * config/arm/constraints.md (Uu): New constraint.
>         * config/arm/arm.md (*arm_movqi_insn): Compute attr "length".


OK.

R.