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

[Chung-Lin Tang <cltang@codesourcery.com>]

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