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

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

[Carrot Wei]

Hi

As described in http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47855, there are
many insn patterns don't compute attribute length correctly. This patch is the
first and simplest part of the fixing.

This patch has been tested on qemu.

thanks
Carrot


ChangeLog:
2011-03-26  Wei Guozhi  <carrot@google.com>

        PR target/47855
        * config/arm/arm.md (arm_cmpsi_insn): Compute attr "length".
        (arm_cond_branch): Likewise.
        (arm_cond_branch_reversed): Likewise.
        (arm_jump): Likewise.
        (push_multi): Likewise.


Index: arm.md
===================================================================
--- arm.md	(revision 171337)
+++ arm.md	(working copy)
@@ -7115,7 +7115,18 @@
   "@
    cmp%?\\t%0, %1
    cmn%?\\t%0, #%n1"
-  [(set_attr "conds" "set")]
+  [(set_attr "conds" "set")
+   (set (attr "length")
+     (if_then_else
+       (and (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		 (eq (symbol_ref "which_alternative") (const_int 0)))
+	    (ior (ne (symbol_ref "REG_P (operands[1])") (const_int 0))
+		(and (ne (symbol_ref "CONST_INT_P (operands[1])") (const_int 0))
+		     (and (ge (symbol_ref "INTVAL (operands[1])") (const_int 0))
+			  (le (symbol_ref "INTVAL (operands[1])")
+			      (const_int 255))))))
+       (const_int 2)
+       (const_int 4)))]
 )

 (define_insn "*cmpsi_shiftsi"
@@ -7286,7 +7297,14 @@
   return \"b%d1\\t%l0\";
   "
   [(set_attr "conds" "use")
-   (set_attr "type" "branch")]
+   (set_attr "type" "branch")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(and (ge (minus (match_dup 0) (pc)) (const_int -250))
+		     (le (minus (match_dup 0) (pc)) (const_int 256))))
+	   (const_int 2)
+	   (const_int 4)))]
 )

 (define_insn "*arm_cond_branch_reversed"
@@ -7305,7 +7323,14 @@
   return \"b%D1\\t%l0\";
   "
   [(set_attr "conds" "use")
-   (set_attr "type" "branch")]
+   (set_attr "type" "branch")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(and (ge (minus (match_dup 0) (pc)) (const_int -250))
+		     (le (minus (match_dup 0) (pc)) (const_int 256))))
+	   (const_int 2)
+	   (const_int 4)))]
 )



@@ -7757,7 +7782,14 @@
     return \"b%?\\t%l0\";
   }
   "
-  [(set_attr "predicable" "yes")]
+  [(set_attr "predicable" "yes")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(and (ge (minus (match_dup 0) (pc)) (const_int -2044))
+		     (le (minus (match_dup 0) (pc)) (const_int 2048))))
+	   (const_int 2)
+	   (const_int 4)))]
 )

 (define_insn "*thumb_jump"
@@ -10256,7 +10288,26 @@

     return \"\";
   }"
-  [(set_attr "type" "store4")]
+  [(set_attr "type" "store4")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(ne (symbol_ref "{
+		    int i, regno, hi_reg;
+		    int num_saves = XVECLEN (operands[2], 0);
+		    regno = REGNO (operands[1]);
+		    hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
+			     && (regno != LR_REGNUM);
+		    for (i = 1; i < num_saves; i++)
+		      {
+			regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0));
+			hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)
+				  && (regno != LR_REGNUM);
+		      }
+		    !hi_reg;    }")
+		  (const_int 0)))
+	   (const_int 2)
+	   (const_int 4)))]
 )

 (define_insn "stack_tie"

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

[Ramana Radhakrishnan]

Hi Carrot,

On 26/03/11 15:14, Carrot Wei wrote:
> Index: arm.md
> ===================================================================
> --- arm.md	(revision 171337)
> +++ arm.md	(working copy)
> @@ -7115,7 +7115,18 @@
>     "@
>      cmp%?\\t%0, %1
>      cmn%?\\t%0, #%n1"
> -  [(set_attr "conds" "set")]
> +  [(set_attr "conds" "set")
> +   (set (attr "length")
> +     (if_then_else
> +       (and (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> +		 (eq (symbol_ref "which_alternative") (const_int 0)))
> +	    (ior (ne (symbol_ref "REG_P (operands[1])") (const_int 0))
> +		(and (ne (symbol_ref "CONST_INT_P (operands[1])") (const_int 0))
> +		     (and (ge (symbol_ref "INTVAL (operands[1])") (const_int 0))
> +			  (le (symbol_ref "INTVAL (operands[1])")
> +			      (const_int 255))))))
> +       (const_int 2)
> +       (const_int 4)))]
>   )

	How about adding an alternative only enabled for T2 that uses the `l' 
constraint and inventing new constraints for some of the constant values 
that are valid for 16 bit instructions since the `I' and `L' constraints 
have different meanings depending on whether TARGET_32BIT is valid or 
not ? We could then set the value of the length attribute accordingly. I 
don't think we can change the meaning of the I and L constraints very 
easily given the amount of churn that might be needed


>
>   (define_insn "*cmpsi_shiftsi"
> @@ -7286,7 +7297,14 @@
>     return \"b%d1\\t%l0\";
>     "
>     [(set_attr "conds" "use")
> -   (set_attr "type" "branch")]
> +   (set_attr "type" "branch")
> +   (set (attr "length")
> +	(if_then_else
> +	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> +		(and (ge (minus (match_dup 0) (pc)) (const_int -250))
> +		     (le (minus (match_dup 0) (pc)) (const_int 256))))
> +	   (const_int 2)
> +	   (const_int 4)))]
>   )

	I don't think this and the other conditional branch instruction changes 
are correct. This could end up being the last instruction in an IT block 
and will automatically end up getting the 32 bit encoding and end up 
causing trouble with the length calculations. Remember the 16 bit 
encoding for the conditional instruction can't be used as the last 
instruction in an IT block.


> @@ -10256,7 +10288,26 @@
>
>       return \"\";
>     }"
> -  [(set_attr "type" "store4")]
> +  [(set_attr "type" "store4")
> +   (set (attr "length")
> +	(if_then_else
> +	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> +		(ne (symbol_ref "{
> +		    int i, regno, hi_reg;
> +		    int num_saves = XVECLEN (operands[2], 0);
> +		    regno = REGNO (operands[1]);
> +		    hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
> +			&&  (regno != LR_REGNUM);
> +		    for (i = 1; i<  num_saves; i++)

	
	(i < num_saves && (hi_reg == 0)) - what's the point of going through 
the register list when hi_reg != 0 in this case ? A comment to explain 
that the length calculation *must* be kept in sync with the template 
above might be useful.

cheers
Ramana

	

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

[Chung-Lin Tang]

On 2011/3/30 06:35 AM, Ramana Radhakrishnan wrote:
> Hi Carrot,
> 
> On 26/03/11 15:14, Carrot Wei wrote:
>> Index: arm.md
>> ===================================================================
>> --- arm.md    (revision 171337)
>> +++ arm.md    (working copy)
>> @@ -7115,7 +7115,18 @@
>>     "@
>>      cmp%?\\t%0, %1
>>      cmn%?\\t%0, #%n1"
>> -  [(set_attr "conds" "set")]
>> +  [(set_attr "conds" "set")
>> +   (set (attr "length")
>> +     (if_then_else
>> +       (and (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
>> +         (eq (symbol_ref "which_alternative") (const_int 0)))
>> +        (ior (ne (symbol_ref "REG_P (operands[1])") (const_int 0))
>> +        (and (ne (symbol_ref "CONST_INT_P (operands[1])") (const_int 0))
>> +             (and (ge (symbol_ref "INTVAL (operands[1])") (const_int 0))
>> +              (le (symbol_ref "INTVAL (operands[1])")
>> +                  (const_int 255))))))
>> +       (const_int 2)
>> +       (const_int 4)))]
>>   )
> 
>     How about adding an alternative only enabled for T2 that uses the
> `l' constraint and inventing new constraints for some of the constant
> values that are valid for 16 bit instructions since the `I' and `L'
> constraints have different meanings depending on whether TARGET_32BIT is
> valid or not ? We could then set the value of the length attribute
> accordingly. I don't think we can change the meaning of the I and L
> constraints very easily given the amount of churn that might be needed
> 
> 
>>
>>   (define_insn "*cmpsi_shiftsi"
>> @@ -7286,7 +7297,14 @@
>>     return \"b%d1\\t%l0\";
>>     "
>>     [(set_attr "conds" "use")
>> -   (set_attr "type" "branch")]
>> +   (set_attr "type" "branch")
>> +   (set (attr "length")
>> +    (if_then_else
>> +       (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
>> +        (and (ge (minus (match_dup 0) (pc)) (const_int -250))
>> +             (le (minus (match_dup 0) (pc)) (const_int 256))))
>> +       (const_int 2)
>> +       (const_int 4)))]
>>   )
> 
>     I don't think this and the other conditional branch instruction
> changes are correct. This could end up being the last instruction in an
> IT block and will automatically end up getting the 32 bit encoding and
> end up causing trouble with the length calculations. Remember the 16 bit
> encoding for the conditional instruction can't be used as the last
> instruction in an IT block.
> 
> 
>> @@ -10256,7 +10288,26 @@
>>
>>       return \"\";
>>     }"
>> -  [(set_attr "type" "store4")]
>> +  [(set_attr "type" "store4")
>> +   (set (attr "length")
>> +    (if_then_else
>> +       (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
>> +        (ne (symbol_ref "{
>> +            int i, regno, hi_reg;
>> +            int num_saves = XVECLEN (operands[2], 0);
>> +            regno = REGNO (operands[1]);
>> +            hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
>> +            &&  (regno != LR_REGNUM);
>> +            for (i = 1; i<  num_saves; i++)
> 
>     
>     (i < num_saves && (hi_reg == 0)) - what's the point of going through
> the register list when hi_reg != 0 in this case ? A comment to explain
> that the length calculation *must* be kept in sync with the template
> above might be useful.

I suggest abstracting all of this Thumb-2 insn length knowledge into a
arm.c function, say "thumb2_16bit_insn_p()" to test for 2-byte patterns.
 Adding more of these quite large pieces of logic into arm.md does not
look very pretty to me, and centralizing it into a C function will also
make it easier to manage the cases.

Chung-Lin

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

[Richard Earnshaw]

On Wed, 2011-03-30 at 12:38 +0800, Chung-Lin Tang wrote:
> On 2011/3/30 06:35 AM, Ramana Radhakrishnan wrote:
> > Hi Carrot,
> > 
> > On 26/03/11 15:14, Carrot Wei wrote:
> >> Index: arm.md
> >> ===================================================================
> >> --- arm.md    (revision 171337)
> >> +++ arm.md    (working copy)
> >> @@ -7115,7 +7115,18 @@
> >>     "@
> >>      cmp%?\\t%0, %1
> >>      cmn%?\\t%0, #%n1"
> >> -  [(set_attr "conds" "set")]
> >> +  [(set_attr "conds" "set")
> >> +   (set (attr "length")
> >> +     (if_then_else
> >> +       (and (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> >> +         (eq (symbol_ref "which_alternative") (const_int 0)))
> >> +        (ior (ne (symbol_ref "REG_P (operands[1])") (const_int 0))
> >> +        (and (ne (symbol_ref "CONST_INT_P (operands[1])") (const_int 0))
> >> +             (and (ge (symbol_ref "INTVAL (operands[1])") (const_int 0))
> >> +              (le (symbol_ref "INTVAL (operands[1])")
> >> +                  (const_int 255))))))
> >> +       (const_int 2)
> >> +       (const_int 4)))]
> >>   )
> > 
> >     How about adding an alternative only enabled for T2 that uses the
> > `l' constraint and inventing new constraints for some of the constant
> > values that are valid for 16 bit instructions since the `I' and `L'
> > constraints have different meanings depending on whether TARGET_32BIT is
> > valid or not ? We could then set the value of the length attribute
> > accordingly. I don't think we can change the meaning of the I and L
> > constraints very easily given the amount of churn that might be needed
> > 
> > 
> >>
> >>   (define_insn "*cmpsi_shiftsi"
> >> @@ -7286,7 +7297,14 @@
> >>     return \"b%d1\\t%l0\";
> >>     "
> >>     [(set_attr "conds" "use")
> >> -   (set_attr "type" "branch")]
> >> +   (set_attr "type" "branch")
> >> +   (set (attr "length")
> >> +    (if_then_else
> >> +       (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> >> +        (and (ge (minus (match_dup 0) (pc)) (const_int -250))
> >> +             (le (minus (match_dup 0) (pc)) (const_int 256))))
> >> +       (const_int 2)
> >> +       (const_int 4)))]
> >>   )
> > 
> >     I don't think this and the other conditional branch instruction
> > changes are correct. This could end up being the last instruction in an
> > IT block and will automatically end up getting the 32 bit encoding and
> > end up causing trouble with the length calculations. Remember the 16 bit
> > encoding for the conditional instruction can't be used as the last
> > instruction in an IT block.
> > 
> > 
> >> @@ -10256,7 +10288,26 @@
> >>
> >>       return \"\";
> >>     }"
> >> -  [(set_attr "type" "store4")]
> >> +  [(set_attr "type" "store4")
> >> +   (set (attr "length")
> >> +    (if_then_else
> >> +       (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> >> +        (ne (symbol_ref "{
> >> +            int i, regno, hi_reg;
> >> +            int num_saves = XVECLEN (operands[2], 0);
> >> +            regno = REGNO (operands[1]);
> >> +            hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
> >> +            &&  (regno != LR_REGNUM);
> >> +            for (i = 1; i<  num_saves; i++)
> > 
> >     
> >     (i < num_saves && (hi_reg == 0)) - what's the point of going through
> > the register list when hi_reg != 0 in this case ? A comment to explain
> > that the length calculation *must* be kept in sync with the template
> > above might be useful.
> 
> I suggest abstracting all of this Thumb-2 insn length knowledge into a
> arm.c function, say "thumb2_16bit_insn_p()" to test for 2-byte patterns.
>  Adding more of these quite large pieces of logic into arm.md does not
> look very pretty to me, and centralizing it into a C function will also
> make it easier to manage the cases.
> 
> Chung-Lin
> 

I think Ramana's suggestion should avoid the need for any arithmetic at
all here.  If the constraints precisely match a 16-bit instruction (and
that alternative is disbled for ARM) then we can say unconditionally
that the length of the insn is 2.

R.

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

[Carrot Wei]

Hi Ramana

On Wed, Mar 30, 2011 at 6:35 AM, Ramana Radhakrishnan
<ramana.radhakrishnan@linaro.org> wrote:
> Hi Carrot,
>
>
>        How about adding an alternative only enabled for T2 that uses the `l'
> constraint and inventing new constraints for some of the constant values
> that are valid for 16 bit instructions since the `I' and `L' constraints
> have different meanings depending on whether TARGET_32BIT is valid or not ?
> We could then set the value of the length attribute accordingly. I don't
> think we can change the meaning of the I and L constraints very easily given
> the amount of churn that might be needed
>
You are right. Now the logic is much clearer by splitting the constraints.
>
>        I don't think this and the other conditional branch instruction
> changes are correct. This could end up being the last instruction in an IT
> block and will automatically end up getting the 32 bit encoding and end up
> causing trouble with the length calculations. Remember the 16 bit encoding
> for the conditional instruction can't be used as the last instruction in an
> IT block.
>
According to arm architecture reference manual, chapter A8.6.16, neither 16 bit
nor 32 bit conditional branch can be used in IT block. Only unconditional branch
can be used as the last instruction in IT block, and it isn't related
to instruction
length. So we don't need to care about branch instruction encoding in IT block.

>
>        (i < num_saves && (hi_reg == 0)) - what's the point of going through
> the register list when hi_reg != 0 in this case ? A comment to explain that
> the length calculation *must* be kept in sync with the template above might
> be useful.
done.

The following patch has been tested on arm qemu.


ChangeLog:
2011-03-31  Wei Guozhi  <carrot@google.com>

        PR target/47855
        * config/arm/arm.md (arm_cmpsi_insn): Compute attr "length".
        (arm_cond_branch): Likewise.
        (arm_cond_branch_reversed): Likewise.
        (arm_jump): Likewise.
        (push_multi): Likewise.
        * config/arm/constraints.md (Py): New constraint.


Index: constraints.md
===================================================================
--- constraints.md	(revision 171337)
+++ constraints.md	(working copy)
@@ -31,7 +31,7 @@
 ;; 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
+;; in Thumb-2 state: Ps, Pt, Pu, Pv, Pw, Px, Py

 ;; The following memory constraints have been used:
 ;; in ARM/Thumb-2 state: Q, Ut, Uv, Uy, Un, Um, Us
@@ -189,6 +189,11 @@
   (and (match_code "const_int")
        (match_test "TARGET_THUMB2 && ival >= -7 && ival <= -1")))

+(define_constraint "Py"
+  "@internal In Thumb-2 state a constant in the range 0 to 255"
+  (and (match_code "const_int")
+       (match_test "TARGET_THUMB2 && ival >= 0 && ival <= 255")))
+
 (define_constraint "G"
  "In ARM/Thumb-2 state a valid FPA immediate constant."
  (and (match_code "const_double")
Index: arm.md
===================================================================
--- arm.md	(revision 171337)
+++ arm.md	(working copy)
@@ -7109,13 +7109,21 @@

 (define_insn "*arm_cmpsi_insn"
   [(set (reg:CC CC_REGNUM)
-	(compare:CC (match_operand:SI 0 "s_register_operand" "r,r")
-		    (match_operand:SI 1 "arm_add_operand"    "rI,L")))]
+	(compare:CC (match_operand:SI 0 "s_register_operand" "l,r,r,r")
+		    (match_operand:SI 1 "arm_add_operand"    "Py,r,I,L")))]
   "TARGET_32BIT"
   "@
    cmp%?\\t%0, %1
+   cmp%?\\t%0, %1
+   cmp%?\\t%0, %1
    cmn%?\\t%0, #%n1"
-  [(set_attr "conds" "set")]
+  [(set_attr "conds" "set")
+   (set (attr "length")
+     (if_then_else
+       (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+	    (lt (symbol_ref "which_alternative") (const_int 2)))
+       (const_int 2)
+       (const_int 4)))]
 )

 (define_insn "*cmpsi_shiftsi"
@@ -7286,7 +7294,14 @@
   return \"b%d1\\t%l0\";
   "
   [(set_attr "conds" "use")
-   (set_attr "type" "branch")]
+   (set_attr "type" "branch")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(and (ge (minus (match_dup 0) (pc)) (const_int -250))
+		     (le (minus (match_dup 0) (pc)) (const_int 256))))
+	   (const_int 2)
+	   (const_int 4)))]
 )

 (define_insn "*arm_cond_branch_reversed"
@@ -7305,7 +7320,14 @@
   return \"b%D1\\t%l0\";
   "
   [(set_attr "conds" "use")
-   (set_attr "type" "branch")]
+   (set_attr "type" "branch")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(and (ge (minus (match_dup 0) (pc)) (const_int -250))
+		     (le (minus (match_dup 0) (pc)) (const_int 256))))
+	   (const_int 2)
+	   (const_int 4)))]
 )



@@ -7757,7 +7779,14 @@
     return \"b%?\\t%l0\";
   }
   "
-  [(set_attr "predicable" "yes")]
+  [(set_attr "predicable" "yes")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(and (ge (minus (match_dup 0) (pc)) (const_int -2044))
+		     (le (minus (match_dup 0) (pc)) (const_int 2048))))
+	   (const_int 2)
+	   (const_int 4)))]
 )

 (define_insn "*thumb_jump"
@@ -10256,7 +10285,29 @@

     return \"\";
   }"
-  [(set_attr "type" "store4")]
+  [(set_attr "type" "store4")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(ne (symbol_ref "{
+		    /* Check if there are any high register (except lr)
+		       references in the list. KEEP the following iteration
+		       in sync with the template above.  */
+		    int i, regno, hi_reg;
+		    int num_saves = XVECLEN (operands[2], 0);
+		    regno = REGNO (operands[1]);
+		    hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
+			     && (regno != LR_REGNUM);
+		    for (i = 1; i < num_saves && !hi_reg; i++)
+		      {
+			regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0));
+			hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)
+				  && (regno != LR_REGNUM);
+		      }
+		    !hi_reg;    }")
+		  (const_int 0)))
+	   (const_int 2)
+	   (const_int 4)))]
 )

 (define_insn "stack_tie"

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

[Ramana Radhakrishnan]

>> Hi Carrot,
>>
>>
>>         How about adding an alternative only enabled for T2 that uses the `l'
>> constraint and inventing new constraints for some of the constant values
>> that are valid for 16 bit instructions since the `I' and `L' constraints
>> have different meanings depending on whether TARGET_32BIT is valid or not ?
>> We could then set the value of the length attribute accordingly. I don't
>> think we can change the meaning of the I and L constraints very easily given
>> the amount of churn that might be needed
>>
> You are right. Now the logic is much clearer by splitting the constraints.

Sorry I wasn't too clear .

What I meant was to use the "enabled" trick and enable this only when 
Thumb2 is enabled in the compiler.

So what you could do instead is add the alternative and then use the 
arch attribute to enable this only for Thumb2.

i.e.

>>   (define_insn "*arm_cmpsi_insn"
>>     [(set (reg:CC CC_REGNUM)
>> -	(compare:CC (match_operand:SI 0 "s_register_operand" "r,r")
>> -		    (match_operand:SI 1 "arm_add_operand"    "rI,L")))]
>> +	(compare:CC (match_operand:SI 0 "s_register_operand" "l,r,r,r")
>> +		    (match_operand:SI 1 "arm_add_operand"    "Py,r,I,L")))]
>>     "TARGET_32BIT"
>>     "@
>>      cmp%?\\t%0, %1
>> +   cmp%?\\t%0, %1
>> +   cmp%?\\t%0, %1
>>      cmn%?\\t%0, #%n1"
>> -  [(set_attr "conds" "set")]
>> +  [(set_attr "conds" "set")

        (set_attr "arch" "t2, any")
        (set_attr "length" "2, 4")]

Does that look any better ? That way you can now safely add options on a 
per arch basis and reduce the amount of complexity for some of these 
length calculations.


>>
>>         I don't think this and the other conditional branch instruction
>> changes are correct. This could end up being the last instruction in an IT
>> block and will automatically end up getting the 32 bit encoding and end up
>> causing trouble with the length calculations. Remember the 16 bit encoding
>> for the conditional instruction can't be used as the last instruction in an
>> IT block.
>>
> According to arm architecture reference manual, chapter A8.6.16, neither 16 bit
> nor 32 bit conditional branch can be used in IT block. Only unconditional branch
> can be used as the last instruction in IT block, and it isn't related
> to instruction
> length. So we don't need to care about branch instruction encoding in IT block.

Yes I think you are right in this case. The reason I think I 
misinterpreted this was ofcourse the fact that conditional branches as 
the last instruction in the ARM-ARM. I don't know why I misread it last 
night.


cheers
Ramana

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

[Carrot Wei]

Hi Ramana

On Thu, Mar 31, 2011 at 11:57 PM, Ramana Radhakrishnan
<ramana.radhakrishnan@linaro.org> wrote:
>>> Hi Carrot,
>>>        How about adding an alternative only enabled for T2 that uses the
>>> `l'
>>> constraint and inventing new constraints for some of the constant values
>>> that are valid for 16 bit instructions since the `I' and `L' constraints
>>> have different meanings depending on whether TARGET_32BIT is valid or not
>>> ?
>>> We could then set the value of the length attribute accordingly. I don't
>>> think we can change the meaning of the I and L constraints very easily
>>> given
>>> the amount of churn that might be needed
>>>
>> You are right. Now the logic is much clearer by splitting the constraints.
>
> Sorry I wasn't too clear .
>
> What I meant was to use the "enabled" trick and enable this only when Thumb2
> is enabled in the compiler.
>
> So what you could do instead is add the alternative and then use the arch
> attribute to enable this only for Thumb2.
>
> i.e.
>
>>>  (define_insn "*arm_cmpsi_insn"
>>>    [(set (reg:CC CC_REGNUM)
>>> -       (compare:CC (match_operand:SI 0 "s_register_operand" "r,r")
>>> -                   (match_operand:SI 1 "arm_add_operand"    "rI,L")))]
>>> +       (compare:CC (match_operand:SI 0 "s_register_operand" "l,r,r,r")
>>> +                   (match_operand:SI 1 "arm_add_operand"
>>>  "Py,r,I,L")))]
>>>    "TARGET_32BIT"
>>>    "@
>>>     cmp%?\\t%0, %1
>>> +   cmp%?\\t%0, %1
>>> +   cmp%?\\t%0, %1
>>>     cmn%?\\t%0, #%n1"
>>> -  [(set_attr "conds" "set")]
>>> +  [(set_attr "conds" "set")
>
>       (set_attr "arch" "t2, any")
>       (set_attr "length" "2, 4")]
>
> Does that look any better ? That way you can now safely add options on a per
> arch basis and reduce the amount of complexity for some of these length
> calculations.
>

Thank you for the knowledge, this is the first time I see an usage of
attribute "arch".
I modified the pattern by enabling the new alternatives for thumb2 only and set
attribute "length" accordingly.

The patch was tested on arm qemu without regression, OK for install?

thanks
Carrot


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

        PR target/47855
        * config/arm/arm.md (arm_cmpsi_insn): Compute attr "length".
        (arm_cond_branch): Likewise.
        (arm_cond_branch_reversed): Likewise.
        (arm_jump): Likewise.
        (push_multi): Likewise.
        * config/arm/constraints.md (Py): New constraint.


Index: constraints.md
===================================================================
--- constraints.md	(revision 171337)
+++ constraints.md	(working copy)
@@ -31,7 +31,7 @@
 ;; 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
+;; in Thumb-2 state: Ps, Pt, Pu, Pv, Pw, Px, Py

 ;; The following memory constraints have been used:
 ;; in ARM/Thumb-2 state: Q, Ut, Uv, Uy, Un, Um, Us
@@ -189,6 +189,11 @@
   (and (match_code "const_int")
        (match_test "TARGET_THUMB2 && ival >= -7 && ival <= -1")))

+(define_constraint "Py"
+  "@internal In Thumb-2 state a constant in the range 0 to 255"
+  (and (match_code "const_int")
+       (match_test "TARGET_THUMB2 && ival >= 0 && ival <= 255")))
+
 (define_constraint "G"
  "In ARM/Thumb-2 state a valid FPA immediate constant."
  (and (match_code "const_double")
Index: arm.md
===================================================================
--- arm.md	(revision 171337)
+++ arm.md	(working copy)
@@ -7109,13 +7109,17 @@

 (define_insn "*arm_cmpsi_insn"
   [(set (reg:CC CC_REGNUM)
-	(compare:CC (match_operand:SI 0 "s_register_operand" "r,r")
-		    (match_operand:SI 1 "arm_add_operand"    "rI,L")))]
+	(compare:CC (match_operand:SI 0 "s_register_operand" "l,r,r,r")
+		    (match_operand:SI 1 "arm_add_operand"    "Py,r,rI,L")))]
   "TARGET_32BIT"
   "@
    cmp%?\\t%0, %1
+   cmp%?\\t%0, %1
+   cmp%?\\t%0, %1
    cmn%?\\t%0, #%n1"
-  [(set_attr "conds" "set")]
+  [(set_attr "conds" "set")
+   (set_attr "arch" "t2,t2,any,any")
+   (set_attr "length" "2,2,4,4")]
 )

 (define_insn "*cmpsi_shiftsi"
@@ -7286,7 +7290,14 @@
   return \"b%d1\\t%l0\";
   "
   [(set_attr "conds" "use")
-   (set_attr "type" "branch")]
+   (set_attr "type" "branch")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(and (ge (minus (match_dup 0) (pc)) (const_int -250))
+		     (le (minus (match_dup 0) (pc)) (const_int 256))))
+	   (const_int 2)
+	   (const_int 4)))]
 )

 (define_insn "*arm_cond_branch_reversed"
@@ -7305,7 +7316,14 @@
   return \"b%D1\\t%l0\";
   "
   [(set_attr "conds" "use")
-   (set_attr "type" "branch")]
+   (set_attr "type" "branch")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(and (ge (minus (match_dup 0) (pc)) (const_int -250))
+		     (le (minus (match_dup 0) (pc)) (const_int 256))))
+	   (const_int 2)
+	   (const_int 4)))]
 )



@@ -7757,7 +7775,14 @@
     return \"b%?\\t%l0\";
   }
   "
-  [(set_attr "predicable" "yes")]
+  [(set_attr "predicable" "yes")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(and (ge (minus (match_dup 0) (pc)) (const_int -2044))
+		     (le (minus (match_dup 0) (pc)) (const_int 2048))))
+	   (const_int 2)
+	   (const_int 4)))]
 )

 (define_insn "*thumb_jump"
@@ -10256,7 +10281,29 @@

     return \"\";
   }"
-  [(set_attr "type" "store4")]
+  [(set_attr "type" "store4")
+   (set (attr "length")
+	(if_then_else
+	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
+		(ne (symbol_ref "{
+		    /* Check if there are any high register (except lr)
+		       references in the list. KEEP the following iteration
+		       in sync with the template above.  */
+		    int i, regno, hi_reg;
+		    int num_saves = XVECLEN (operands[2], 0);
+		    regno = REGNO (operands[1]);
+		    hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
+			     && (regno != LR_REGNUM);
+		    for (i = 1; i < num_saves && !hi_reg; i++)
+		      {
+			regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0));
+			hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)
+				  && (regno != LR_REGNUM);
+		      }
+		    !hi_reg;    }")
+		  (const_int 0)))
+	   (const_int 2)
+	   (const_int 4)))]
 )

 (define_insn "stack_tie"

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

[Ramana Radhakrishnan]

>
> The patch was tested on arm qemu without regression, OK for install?

Ok if no regressions.

cheers
Ramana

>
> thanks
> Carrot
>
>
> ChangeLog:
> 2011-04-01  Wei Guozhi<carrot@google.com>
>
>          PR target/47855
>          * config/arm/arm.md (arm_cmpsi_insn): Compute attr "length".
>          (arm_cond_branch): Likewise.
>          (arm_cond_branch_reversed): Likewise.
>          (arm_jump): Likewise.
>          (push_multi): Likewise.
>          * config/arm/constraints.md (Py): New constraint.
>
>
> Index: constraints.md
> ===================================================================
> --- constraints.md	(revision 171337)
> +++ constraints.md	(working copy)
> @@ -31,7 +31,7 @@
>   ;; 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
> +;; in Thumb-2 state: Ps, Pt, Pu, Pv, Pw, Px, Py
>
>   ;; The following memory constraints have been used:
>   ;; in ARM/Thumb-2 state: Q, Ut, Uv, Uy, Un, Um, Us
> @@ -189,6 +189,11 @@
>     (and (match_code "const_int")
>          (match_test "TARGET_THUMB2&&  ival>= -7&&  ival<= -1")))
>
> +(define_constraint "Py"
> +  "@internal In Thumb-2 state a constant in the range 0 to 255"
> +  (and (match_code "const_int")
> +       (match_test "TARGET_THUMB2&&  ival>= 0&&  ival<= 255")))
> +
>   (define_constraint "G"
>    "In ARM/Thumb-2 state a valid FPA immediate constant."
>    (and (match_code "const_double")
> Index: arm.md
> ===================================================================
> --- arm.md	(revision 171337)
> +++ arm.md	(working copy)
> @@ -7109,13 +7109,17 @@
>
>   (define_insn "*arm_cmpsi_insn"
>     [(set (reg:CC CC_REGNUM)
> -	(compare:CC (match_operand:SI 0 "s_register_operand" "r,r")
> -		    (match_operand:SI 1 "arm_add_operand"    "rI,L")))]
> +	(compare:CC (match_operand:SI 0 "s_register_operand" "l,r,r,r")
> +		    (match_operand:SI 1 "arm_add_operand"    "Py,r,rI,L")))]
>     "TARGET_32BIT"
>     "@
>      cmp%?\\t%0, %1
> +   cmp%?\\t%0, %1
> +   cmp%?\\t%0, %1
>      cmn%?\\t%0, #%n1"
> -  [(set_attr "conds" "set")]
> +  [(set_attr "conds" "set")
> +   (set_attr "arch" "t2,t2,any,any")
> +   (set_attr "length" "2,2,4,4")]
>   )
>
>   (define_insn "*cmpsi_shiftsi"
> @@ -7286,7 +7290,14 @@
>     return \"b%d1\\t%l0\";
>     "
>     [(set_attr "conds" "use")
> -   (set_attr "type" "branch")]
> +   (set_attr "type" "branch")
> +   (set (attr "length")
> +	(if_then_else
> +	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> +		(and (ge (minus (match_dup 0) (pc)) (const_int -250))
> +		     (le (minus (match_dup 0) (pc)) (const_int 256))))
> +	   (const_int 2)
> +	   (const_int 4)))]
>   )
>
>   (define_insn "*arm_cond_branch_reversed"
> @@ -7305,7 +7316,14 @@
>     return \"b%D1\\t%l0\";
>     "
>     [(set_attr "conds" "use")
> -   (set_attr "type" "branch")]
> +   (set_attr "type" "branch")
> +   (set (attr "length")
> +	(if_then_else
> +	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> +		(and (ge (minus (match_dup 0) (pc)) (const_int -250))
> +		     (le (minus (match_dup 0) (pc)) (const_int 256))))
> +	   (const_int 2)
> +	   (const_int 4)))]
>   )
>
>
>
> @@ -7757,7 +7775,14 @@
>       return \"b%?\\t%l0\";
>     }
>     "
> -  [(set_attr "predicable" "yes")]
> +  [(set_attr "predicable" "yes")
> +   (set (attr "length")
> +	(if_then_else
> +	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> +		(and (ge (minus (match_dup 0) (pc)) (const_int -2044))
> +		     (le (minus (match_dup 0) (pc)) (const_int 2048))))
> +	   (const_int 2)
> +	   (const_int 4)))]
>   )
>
>   (define_insn "*thumb_jump"
> @@ -10256,7 +10281,29 @@
>
>       return \"\";
>     }"
> -  [(set_attr "type" "store4")]
> +  [(set_attr "type" "store4")
> +   (set (attr "length")
> +	(if_then_else
> +	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> +		(ne (symbol_ref "{
> +		    /* Check if there are any high register (except lr)
> +		       references in the list. KEEP the following iteration
> +		       in sync with the template above.  */
> +		    int i, regno, hi_reg;
> +		    int num_saves = XVECLEN (operands[2], 0);
> +		    regno = REGNO (operands[1]);
> +		    hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
> +			&&  (regno != LR_REGNUM);
> +		    for (i = 1; i<  num_saves&&  !hi_reg; i++)
> +		      {
> +			regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0));
> +			hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)
> +				&&  (regno != LR_REGNUM);
> +		      }
> +		    !hi_reg;    }")
> +		  (const_int 0)))
> +	   (const_int 2)
> +	   (const_int 4)))]
>   )
>
>   (define_insn "stack_tie"

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

[Carrot Wei]

Committed as r172017.

thanks
Carrot

On Tue, Apr 5, 2011 at 9:32 PM, Ramana Radhakrishnan
<ramana.radhakrishnan@linaro.org> wrote:
>
>>
>> The patch was tested on arm qemu without regression, OK for install?
>
> Ok if no regressions.
>
> cheers
> Ramana
>

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

[Richard Sandiford]

Hi Carrot,

Sorry if this has already been reported, but the patch breaks bootstrap
of arm-linux-gnueabi (or cross builds with --enable-werror).  The problem
is that this...

Carrot Wei <carrot@google.com> writes:
> @@ -10256,7 +10281,29 @@
>
>      return \"\";
>    }"
> -  [(set_attr "type" "store4")]
> +  [(set_attr "type" "store4")
> +   (set (attr "length")
> +	(if_then_else
> +	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> +		(ne (symbol_ref "{
> +		    /* Check if there are any high register (except lr)
> +		       references in the list. KEEP the following iteration
> +		       in sync with the template above.  */
> +		    int i, regno, hi_reg;
> +		    int num_saves = XVECLEN (operands[2], 0);
> +		    regno = REGNO (operands[1]);
> +		    hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
> +			     && (regno != LR_REGNUM);
> +		    for (i = 1; i < num_saves && !hi_reg; i++)
> +		      {
> +			regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0));
> +			hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)
> +				  && (regno != LR_REGNUM);
> +		      }
> +		    !hi_reg;    }")
> +		  (const_int 0)))
> +	   (const_int 2)
> +	   (const_int 4)))]

uses a statement expression -- i.e. ({ code; result; }) -- which is
a GNU extension.

I suppose a simple fix would be to put the code into an arm.c function.

Richard

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

[Carrot Wei]

On Thu, Apr 7, 2011 at 5:31 PM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Hi Carrot,
>
> Sorry if this has already been reported, but the patch breaks bootstrap
> of arm-linux-gnueabi (or cross builds with --enable-werror).  The problem
> is that this...
>
> uses a statement expression -- i.e. ({ code; result; }) -- which is
> a GNU extension.
>
> I suppose a simple fix would be to put the code into an arm.c function.
>

Thank you for the report, I will add this fix to the second part of the patch.

Carrot

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

[Ramana Radhakrishnan]

On 07/04/11 12:08, Carrot Wei wrote:
> On Thu, Apr 7, 2011 at 5:31 PM, Richard Sandiford
> <richard.sandiford@linaro.org>  wrote:
>> Hi Carrot,
>>
>> Sorry if this has already been reported, but the patch breaks bootstrap
>> of arm-linux-gnueabi (or cross builds with --enable-werror).  The problem
>> is that this...
>>
>> uses a statement expression -- i.e. ({ code; result; }) -- which is
>> a GNU extension.
>>
>> I suppose a simple fix would be to put the code into an arm.c function.
>>
>
> Thank you for the report, I will add this fix to the second part of the patch.

It would be worth unbreaking bootstrap as a separate patch and not 
conflating it with a different set of improvements.

Ramana

>
> Carrot

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

[Carrot Wei]

Hi

This patch moves the length computation of push_multi into a C
function to avoid the usage of GNU extension.

Tested on qemu without regression. OK to install?

thanks
Carrot

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

        PR target/47855
        * config/arm/arm-protos.h (arm_attr_length_push_multi): New prototype.
        * config/arm/arm.c (arm_attr_length_push_multi): New function.
        * config/arm/arm.md (*push_multi): Change the length computation to
        call a C function.


Index: arm.c
===================================================================
--- arm.c	(revision 172158)
+++ arm.c	(working copy)
@@ -23696,4 +23696,30 @@ arm_preferred_rename_class (reg_class_t
     return NO_REGS;
 }

+/* Compute the atrribute "length" of insn "*push_multi".
+   So this function MUST be kept in sync with that insn pattern.  */
+int
+arm_attr_length_push_multi(rtx parallel_op, rtx first_op)
+{
+  int i, regno, hi_reg;
+  int num_saves = XVECLEN (parallel_op, 0);
+
+  /* ARM mode.  */
+  if (TARGET_ARM)
+    return 4;
+
+  /* Thumb2 mode.  */
+  regno = REGNO (first_op);
+  hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS) && (regno != LR_REGNUM);
+  for (i = 1; i < num_saves && !hi_reg; i++)
+    {
+      regno = REGNO (XEXP (XVECEXP (parallel_op, 0, i), 0));
+      hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS) && (regno != LR_REGNUM);
+    }
+
+  if (!hi_reg)
+    return 2;
+  return 4;
+}
+
 #include "gt-arm.h"
Index: arm-protos.h
===================================================================
--- arm-protos.h	(revision 172158)
+++ arm-protos.h	(working copy)
@@ -152,6 +152,7 @@ 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_push_multi(rtx, rtx);

 #if defined TREE_CODE
 extern void arm_init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx, tree);
Index: arm.md
===================================================================
--- arm.md	(revision 172158)
+++ arm.md	(working copy)
@@ -10290,27 +10290,7 @@
   }"
   [(set_attr "type" "store4")
    (set (attr "length")
-	(if_then_else
-	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
-		(ne (symbol_ref "{
-		    /* Check if there are any high register (except lr)
-		       references in the list. KEEP the following iteration
-		       in sync with the template above.  */
-		    int i, regno, hi_reg;
-		    int num_saves = XVECLEN (operands[2], 0);
-		    regno = REGNO (operands[1]);
-		    hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
-			     && (regno != LR_REGNUM);
-		    for (i = 1; i < num_saves && !hi_reg; i++)
-		      {
-			regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0));
-			hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)
-				  && (regno != LR_REGNUM);
-		      }
-		    !hi_reg;    }")
-		  (const_int 0)))
-	   (const_int 2)
-	   (const_int 4)))]
+	(symbol_ref "arm_attr_length_push_multi (operands[2], operands[1])"))]
 )

 (define_insn "stack_tie"

On Thu, Apr 7, 2011 at 7:30 PM, Ramana Radhakrishnan
<ramana.radhakrishnan@linaro.org> wrote:
> On 07/04/11 12:08, Carrot Wei wrote:
>>
>> On Thu, Apr 7, 2011 at 5:31 PM, Richard Sandiford
>> <richard.sandiford@linaro.org>  wrote:
>>>
>>> Hi Carrot,
>>>
>>> Sorry if this has already been reported, but the patch breaks bootstrap
>>> of arm-linux-gnueabi (or cross builds with --enable-werror).  The problem
>>> is that this...
>>>
>>> uses a statement expression -- i.e. ({ code; result; }) -- which is
>>> a GNU extension.
>>>
>>> I suppose a simple fix would be to put the code into an arm.c function.
>>>
>>
>> Thank you for the report, I will add this fix to the second part of the
>> patch.
>
> It would be worth unbreaking bootstrap as a separate patch and not
> conflating it with a different set of improvements.
>
> Ramana
>
>>
>> Carrot
>
>

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

[Ramana Radhakrishnan]

On 08/04/11 08:36, Carrot Wei wrote:
> Hi
>
> This patch moves the length computation of push_multi into a C
> function to avoid the usage of GNU extension.

Please put the comment regarding the calculation of the length attribute 
along with the pattern in thumb2.md as well.

>
> Tested on qemu without regression. OK to install?

Ok with that change.

Thanks for fixing this up.

cheers
Ramana
>
> thanks
> Carrot
>
> ChangeLog:
> 2011-04-08  Wei Guozhi<carrot@google.com>
>
>          PR target/47855
>          * config/arm/arm-protos.h (arm_attr_length_push_multi): New prototype.
>          * config/arm/arm.c (arm_attr_length_push_multi): New function.
>          * config/arm/arm.md (*push_multi): Change the length computation to
>          call a C function.
>
>
> Index: arm.c
> ===================================================================
> --- arm.c	(revision 172158)
> +++ arm.c	(working copy)
> @@ -23696,4 +23696,30 @@ arm_preferred_rename_class (reg_class_t
>       return NO_REGS;
>   }
>
> +/* Compute the atrribute "length" of insn "*push_multi".
> +   So this function MUST be kept in sync with that insn pattern.  */
> +int
> +arm_attr_length_push_multi(rtx parallel_op, rtx first_op)
> +{
> +  int i, regno, hi_reg;
> +  int num_saves = XVECLEN (parallel_op, 0);
> +
> +  /* ARM mode.  */
> +  if (TARGET_ARM)
> +    return 4;
> +
> +  /* Thumb2 mode.  */
> +  regno = REGNO (first_op);
> +  hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)&&  (regno != LR_REGNUM);
> +  for (i = 1; i<  num_saves&&  !hi_reg; i++)
> +    {
> +      regno = REGNO (XEXP (XVECEXP (parallel_op, 0, i), 0));
> +      hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)&&  (regno != LR_REGNUM);
> +    }
> +
> +  if (!hi_reg)
> +    return 2;
> +  return 4;
> +}
> +
>   #include "gt-arm.h"
> Index: arm-protos.h
> ===================================================================
> --- arm-protos.h	(revision 172158)
> +++ arm-protos.h	(working copy)
> @@ -152,6 +152,7 @@ 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_push_multi(rtx, rtx);
>
>   #if defined TREE_CODE
>   extern void arm_init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx, tree);
> Index: arm.md
> ===================================================================
> --- arm.md	(revision 172158)
> +++ arm.md	(working copy)
> @@ -10290,27 +10290,7 @@
>     }"
>     [(set_attr "type" "store4")
>      (set (attr "length")
> -	(if_then_else
> -	   (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
> -		(ne (symbol_ref "{
> -		    /* Check if there are any high register (except lr)
> -		       references in the list. KEEP the following iteration
> -		       in sync with the template above.  */
> -		    int i, regno, hi_reg;
> -		    int num_saves = XVECLEN (operands[2], 0);
> -		    regno = REGNO (operands[1]);
> -		    hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
> -			&&  (regno != LR_REGNUM);
> -		    for (i = 1; i<  num_saves&&  !hi_reg; i++)
> -		      {
> -			regno = REGNO (XEXP (XVECEXP (operands[2], 0, i), 0));
> -			hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)
> -				&&  (regno != LR_REGNUM);
> -		      }
> -		    !hi_reg;    }")
> -		  (const_int 0)))
> -	   (const_int 2)
> -	   (const_int 4)))]
> +	(symbol_ref "arm_attr_length_push_multi (operands[2], operands[1])"))]
>   )
>
>   (define_insn "stack_tie"
>
> On Thu, Apr 7, 2011 at 7:30 PM, Ramana Radhakrishnan
> <ramana.radhakrishnan@linaro.org>  wrote:
>> On 07/04/11 12:08, Carrot Wei wrote:
>>>
>>> On Thu, Apr 7, 2011 at 5:31 PM, Richard Sandiford
>>> <richard.sandiford@linaro.org>    wrote:
>>>>
>>>> Hi Carrot,
>>>>
>>>> Sorry if this has already been reported, but the patch breaks bootstrap
>>>> of arm-linux-gnueabi (or cross builds with --enable-werror).  The problem
>>>> is that this...
>>>>
>>>> uses a statement expression -- i.e. ({ code; result; }) -- which is
>>>> a GNU extension.
>>>>
>>>> I suppose a simple fix would be to put the code into an arm.c function.
>>>>
>>>
>>> Thank you for the report, I will add this fix to the second part of the
>>> patch.
>>
>> It would be worth unbreaking bootstrap as a separate patch and not
>> conflating it with a different set of improvements.
>>
>> Ramana
>>
>>>
>>> Carrot
>>
>>

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

[Carrot Wei]

Sorry, which pattern in thumb2.md?

thanks
Carrot

On Fri, Apr 8, 2011 at 4:35 PM, Ramana Radhakrishnan
<ramana.radhakrishnan@linaro.org> wrote:
> On 08/04/11 08:36, Carrot Wei wrote:
>>
>> Hi
>>
>> This patch moves the length computation of push_multi into a C
>> function to avoid the usage of GNU extension.
>
> Please put the comment regarding the calculation of the length attribute
> along with the pattern in thumb2.md as well.
>
>>
>> Tested on qemu without regression. OK to install?
>
> Ok with that change.
>
> Thanks for fixing this up.
>
> cheers
> Ramana
>>
>> thanks
>> Carrot
>>
>> ChangeLog:
>> 2011-04-08  Wei Guozhi<carrot@google.com>
>>
>>         PR target/47855
>>         * config/arm/arm-protos.h (arm_attr_length_push_multi): New
>> prototype.
>>         * config/arm/arm.c (arm_attr_length_push_multi): New function.
>>         * config/arm/arm.md (*push_multi): Change the length computation
>> to
>>         call a C function.
>>
>>
>> Index: arm.c
>> ===================================================================
>> --- arm.c       (revision 172158)
>> +++ arm.c       (working copy)
>> @@ -23696,4 +23696,30 @@ arm_preferred_rename_class (reg_class_t
>>      return NO_REGS;
>>  }
>>
>> +/* Compute the atrribute "length" of insn "*push_multi".
>> +   So this function MUST be kept in sync with that insn pattern.  */
>> +int
>> +arm_attr_length_push_multi(rtx parallel_op, rtx first_op)
>> +{
>> +  int i, regno, hi_reg;
>> +  int num_saves = XVECLEN (parallel_op, 0);
>> +
>> +  /* ARM mode.  */
>> +  if (TARGET_ARM)
>> +    return 4;
>> +
>> +  /* Thumb2 mode.  */
>> +  regno = REGNO (first_op);
>> +  hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)&&  (regno != LR_REGNUM);
>> +  for (i = 1; i<  num_saves&&  !hi_reg; i++)
>> +    {
>> +      regno = REGNO (XEXP (XVECEXP (parallel_op, 0, i), 0));
>> +      hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)&&  (regno !=
>> LR_REGNUM);
>> +    }
>> +
>> +  if (!hi_reg)
>> +    return 2;
>> +  return 4;
>> +}
>> +
>>  #include "gt-arm.h"
>> Index: arm-protos.h
>> ===================================================================
>> --- arm-protos.h        (revision 172158)
>> +++ arm-protos.h        (working copy)
>> @@ -152,6 +152,7 @@ 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_push_multi(rtx, rtx);
>>
>>  #if defined TREE_CODE
>>  extern void arm_init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx,
>> tree);
>> Index: arm.md
>> ===================================================================
>> --- arm.md      (revision 172158)
>> +++ arm.md      (working copy)
>> @@ -10290,27 +10290,7 @@
>>    }"
>>    [(set_attr "type" "store4")
>>     (set (attr "length")
>> -       (if_then_else
>> -          (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
>> -               (ne (symbol_ref "{
>> -                   /* Check if there are any high register (except lr)
>> -                      references in the list. KEEP the following
>> iteration
>> -                      in sync with the template above.  */
>> -                   int i, regno, hi_reg;
>> -                   int num_saves = XVECLEN (operands[2], 0);
>> -                   regno = REGNO (operands[1]);
>> -                   hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
>> -                       &&  (regno != LR_REGNUM);
>> -                   for (i = 1; i<  num_saves&&  !hi_reg; i++)
>> -                     {
>> -                       regno = REGNO (XEXP (XVECEXP (operands[2], 0, i),
>> 0));
>> -                       hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)
>> -                               &&  (regno != LR_REGNUM);
>> -                     }
>> -                   !hi_reg;    }")
>> -                 (const_int 0)))
>> -          (const_int 2)
>> -          (const_int 4)))]
>> +       (symbol_ref "arm_attr_length_push_multi (operands[2],
>> operands[1])"))]
>>  )
>>
>>  (define_insn "stack_tie"
>>
>> On Thu, Apr 7, 2011 at 7:30 PM, Ramana Radhakrishnan
>> <ramana.radhakrishnan@linaro.org>  wrote:
>>>
>>> On 07/04/11 12:08, Carrot Wei wrote:
>>>>
>>>> On Thu, Apr 7, 2011 at 5:31 PM, Richard Sandiford
>>>> <richard.sandiford@linaro.org>    wrote:
>>>>>
>>>>> Hi Carrot,
>>>>>
>>>>> Sorry if this has already been reported, but the patch breaks bootstrap
>>>>> of arm-linux-gnueabi (or cross builds with --enable-werror).  The
>>>>> problem
>>>>> is that this...
>>>>>
>>>>> uses a statement expression -- i.e. ({ code; result; }) -- which is
>>>>> a GNU extension.
>>>>>
>>>>> I suppose a simple fix would be to put the code into an arm.c function.
>>>>>
>>>>
>>>> Thank you for the report, I will add this fix to the second part of the
>>>> patch.
>>>
>>> It would be worth unbreaking bootstrap as a separate patch and not
>>> conflating it with a different set of improvements.
>>>
>>> Ramana
>>>
>>>>
>>>> Carrot
>>>
>>>
>
>

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

[Ramana Radhakrishnan]

On 8 April 2011 09:52, Carrot Wei <carrot@google.com> wrote:
> Sorry, which pattern in thumb2.md?

Sorry, I meant to say push_multi in arm.md . Not enough coffee yet this morning.

cheers
Ramana
>
> thanks
> Carrot
>
> On Fri, Apr 8, 2011 at 4:35 PM, Ramana Radhakrishnan
> <ramana.radhakrishnan@linaro.org> wrote:
>> On 08/04/11 08:36, Carrot Wei wrote:
>>>
>>> Hi
>>>
>>> This patch moves the length computation of push_multi into a C
>>> function to avoid the usage of GNU extension.
>>
>> Please put the comment regarding the calculation of the length attribute
>> along with the pattern in thumb2.md as well.
>>
>>>
>>> Tested on qemu without regression. OK to install?
>>
>> Ok with that change.
>>
>> Thanks for fixing this up.
>>
>> cheers
>> Ramana
>>>
>>> thanks
>>> Carrot
>>>
>>> ChangeLog:
>>> 2011-04-08  Wei Guozhi<carrot@google.com>
>>>
>>>         PR target/47855
>>>         * config/arm/arm-protos.h (arm_attr_length_push_multi): New
>>> prototype.
>>>         * config/arm/arm.c (arm_attr_length_push_multi): New function.
>>>         * config/arm/arm.md (*push_multi): Change the length computation
>>> to
>>>         call a C function.
>>>
>>>
>>> Index: arm.c
>>> ===================================================================
>>> --- arm.c       (revision 172158)
>>> +++ arm.c       (working copy)
>>> @@ -23696,4 +23696,30 @@ arm_preferred_rename_class (reg_class_t
>>>      return NO_REGS;
>>>  }
>>>
>>> +/* Compute the atrribute "length" of insn "*push_multi".
>>> +   So this function MUST be kept in sync with that insn pattern.  */
>>> +int
>>> +arm_attr_length_push_multi(rtx parallel_op, rtx first_op)
>>> +{
>>> +  int i, regno, hi_reg;
>>> +  int num_saves = XVECLEN (parallel_op, 0);
>>> +
>>> +  /* ARM mode.  */
>>> +  if (TARGET_ARM)
>>> +    return 4;
>>> +
>>> +  /* Thumb2 mode.  */
>>> +  regno = REGNO (first_op);
>>> +  hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)&&  (regno != LR_REGNUM);
>>> +  for (i = 1; i<  num_saves&&  !hi_reg; i++)
>>> +    {
>>> +      regno = REGNO (XEXP (XVECEXP (parallel_op, 0, i), 0));
>>> +      hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)&&  (regno !=
>>> LR_REGNUM);
>>> +    }
>>> +
>>> +  if (!hi_reg)
>>> +    return 2;
>>> +  return 4;
>>> +}
>>> +
>>>  #include "gt-arm.h"
>>> Index: arm-protos.h
>>> ===================================================================
>>> --- arm-protos.h        (revision 172158)
>>> +++ arm-protos.h        (working copy)
>>> @@ -152,6 +152,7 @@ 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_push_multi(rtx, rtx);
>>>
>>>  #if defined TREE_CODE
>>>  extern void arm_init_cumulative_args (CUMULATIVE_ARGS *, tree, rtx,
>>> tree);
>>> Index: arm.md
>>> ===================================================================
>>> --- arm.md      (revision 172158)
>>> +++ arm.md      (working copy)
>>> @@ -10290,27 +10290,7 @@
>>>    }"
>>>    [(set_attr "type" "store4")
>>>     (set (attr "length")
>>> -       (if_then_else
>>> -          (and (ne (symbol_ref "TARGET_THUMB2") (const_int 0))
>>> -               (ne (symbol_ref "{
>>> -                   /* Check if there are any high register (except lr)
>>> -                      references in the list. KEEP the following
>>> iteration
>>> -                      in sync with the template above.  */
>>> -                   int i, regno, hi_reg;
>>> -                   int num_saves = XVECLEN (operands[2], 0);
>>> -                   regno = REGNO (operands[1]);
>>> -                   hi_reg = (REGNO_REG_CLASS (regno) == HI_REGS)
>>> -                       &&  (regno != LR_REGNUM);
>>> -                   for (i = 1; i<  num_saves&&  !hi_reg; i++)
>>> -                     {
>>> -                       regno = REGNO (XEXP (XVECEXP (operands[2], 0, i),
>>> 0));
>>> -                       hi_reg |= (REGNO_REG_CLASS (regno) == HI_REGS)
>>> -                               &&  (regno != LR_REGNUM);
>>> -                     }
>>> -                   !hi_reg;    }")
>>> -                 (const_int 0)))
>>> -          (const_int 2)
>>> -          (const_int 4)))]
>>> +       (symbol_ref "arm_attr_length_push_multi (operands[2],
>>> operands[1])"))]
>>>  )
>>>
>>>  (define_insn "stack_tie"
>>>
>>> On Thu, Apr 7, 2011 at 7:30 PM, Ramana Radhakrishnan
>>> <ramana.radhakrishnan@linaro.org>  wrote:
>>>>
>>>> On 07/04/11 12:08, Carrot Wei wrote:
>>>>>
>>>>> On Thu, Apr 7, 2011 at 5:31 PM, Richard Sandiford
>>>>> <richard.sandiford@linaro.org>    wrote:
>>>>>>
>>>>>> Hi Carrot,
>>>>>>
>>>>>> Sorry if this has already been reported, but the patch breaks bootstrap
>>>>>> of arm-linux-gnueabi (or cross builds with --enable-werror).  The
>>>>>> problem
>>>>>> is that this...
>>>>>>
>>>>>> uses a statement expression -- i.e. ({ code; result; }) -- which is
>>>>>> a GNU extension.
>>>>>>
>>>>>> I suppose a simple fix would be to put the code into an arm.c function.
>>>>>>
>>>>>
>>>>> Thank you for the report, I will add this fix to the second part of the
>>>>> patch.
>>>>
>>>> It would be worth unbreaking bootstrap as a separate patch and not
>>>> conflating it with a different set of improvements.
>>>>
>>>> Ramana
>>>>
>>>>>
>>>>> Carrot
>>>>
>>>>
>>
>>
>