Re: [PATCH, ARM] Alternatives and type attributes fixes.

[Yvan Roux <yvan dot roux at linaro dot org>]

On 27 April 2015 at 15:20, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
> Hi Yvan,
>
>
> On 27/04/15 13:25, Yvan Roux wrote:
>>
>> Hi,
>>
>> This is a follow-up of PR64208 where an LRA loop was due to redundancy
>> in insn's alternatives.  I've checked all the insns in the arm backend
>> to avoid latent problems and this patch fixes the issues I've spotted.
>>
>> Only thumb2_movsicc_insn contained duplicated alternatives, and the
>> rest of the changes are related to the type attribute, which were not
>> accurate or used accordingly to their definition.  Notice that the
>> modifications have only a limited impact as in most of the pipeline
>> descriptions, multiple/mov_reg/alu_reg/.. types are used the same way,
>> only cortex a8 seems to have a real difference between alu or multiple
>> instruction and mov.
>>
>> Bootstrapped and regtested on arm-linux-gnueabihf.  Ok for trunk ?
>>
>> Thanks,
>> Yvan
>>
>> 2015-04-27  Yvan Roux<yvan.roux@linaro.org>
>>
>>      * config/arm/arm.mb (*arm_movt): Fix type attribute.
>>      (*movsi_compare0): Likewise.
>>      (*cmpsi_shiftsi): Likewise.
>>      (*cmpsi_shiftsi_swp): Likewise.
>>      (*movsicc_insn): Likewise.
>>      (*cond_move): Likewise.
>>      (*if_plus_move): Likewise.
>>      (*if_move_plus): Likewise.
>>      (*if_arith_move): Likewise.
>>      (*if_move_arith): Likewise.
>>      (*if_shift_move): Likewise.
>>      (*if_move_shift): Likewise.
>>      * config/arm/thumb2.md (*thumb2_movsi_insn): Fix type attribute.
>>      (*thumb2_movsicc_insn): Fix alternative redundancy.
>>      (*thumb2_addsi_short): Split register and immediate alternatives.
>>      (thumb2_addsi3_compare0): Likewise.
>>
>> insn.diff
>>
>>
>> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
>> index 164ac13..ee23deb 100644
>> --- a/gcc/config/arm/arm.md
>> +++ b/gcc/config/arm/arm.md
>> @@ -5631,7 +5631,7 @@
>>     [(set_attr "predicable" "yes")
>>      (set_attr "predicable_short_it" "no")
>>      (set_attr "length" "4")
>> -   (set_attr "type" "mov_imm")]
>> +   (set_attr "type" "alu_sreg")]
>>   )
>
>
> For some context, this is the *arm_movt pattern that generates
> a movt instruction. The documentation in types.md says:
> "This excludes MOV and MVN but includes MOVT." So using alu_sreg
> is correct according to that logic.
> However, don't you want to also update the arm_movtas_ze pattern
> that generates movt but erroneously has the type 'mov_imm'?

Ha, yes I missed this one ! I'll will update it.

>>     (define_insn "*arm_movsi_insn"
>> @@ -5919,7 +5919,7 @@
>>      cmp%?\\t%0, #0
>>      sub%.\\t%0, %1, #0"
>>     [(set_attr "conds" "set")
>> -   (set_attr "type" "alus_imm,alus_imm")]
>> +   (set_attr "type" "alus_sreg,alus_sreg")]
>>   )
>
>
> Why change that? They are instructions with immediate operands
> so alus_imm should be gorrect?

Oups, I certainly misread #0 and %0 this one is correct.

>> @@ -486,12 +486,12 @@
>>   )
>>     (define_insn_and_split "*thumb2_movsicc_insn"
>> -  [(set (match_operand:SI 0 "s_register_operand"
>> "=l,l,r,r,r,r,r,r,r,r,r")
>> +  [(set (match_operand:SI 0 "s_register_operand"
>> "=l,l,r,r,r,r,r,r,r,r,r,r")
>>         (if_then_else:SI
>>          (match_operator 3 "arm_comparison_operator"
>>           [(match_operand 4 "cc_register" "") (const_int 0)])
>> -        (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,rI,rI,K
>> ,K,r")
>> -        (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,K
>> ,rI,K,r")))]
>> +        (match_operand:SI 1 "arm_not_operand" "0 ,lPy,0 ,0,rI,K,I ,r,rI,K
>> ,K,r")
>> +        (match_operand:SI 2 "arm_not_operand" "lPy,0 ,rI,K,0 ,0,rI,I,K
>> ,rI,K,r")))]
>>     "TARGET_THUMB2"
>>     "@
>>      it\\t%D3\;mov%D3\\t%0, %2
>> @@ -504,12 +504,14 @@
>>      #
>>      #
>>      #
>> +   #
>>      #"
>>      ; alt 6: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>> -   ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
>> -   ; alt 8: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
>> -   ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
>> -   ; alt 10: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>> +   ; alt 7: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>> +   ; alt 8: ite\\t%d3\;mov%d3\\t%0, %1\;mvn%D3\\t%0, #%B2
>> +   ; alt 9: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mov%D3\\t%0, %2
>> +   ; alt 10: ite\\t%d3\;mvn%d3\\t%0, #%B1\;mvn%D3\\t%0, #%B2
>> +   ; alt 11: ite\\t%d3\;mov%d3\\t%0, %1\;mov%D3\\t%0, %2
>>     "&& reload_completed"
>>     [(const_int 0)]
>>     {
>> @@ -540,8 +542,8 @@
>>                                                  operands[2])));
>>       DONE;
>>     }
>> -  [(set_attr "length" "4,4,6,6,6,6,10,10,10,10,6")
>> -   (set_attr "enabled_for_depr_it" "yes,yes,no,no,no,no,no,no,no,no,yes")
>> +  [(set_attr "length" "4,4,6,6,6,6,10,8,10,10,10,6")
>> +   (set_attr "enabled_for_depr_it"
>> "yes,yes,no,no,no,no,no,no,no,no,no,yes")
>>      (set_attr "conds" "use")
>>      (set_attr "type" "multiple")]
>>   )
>
>
> So, I think here for the first 6 alternatives we can give it a more specific
> type,
> like mov_immm or mov_reg, since you're cleaning this stuff up. The
> instructions in
> those alternatives are just a mov instruction enclosed in an IT block, so
> they always
> have to stick together anyway.

OK I'll change that.

>> @@ -1161,9 +1163,9 @@
>>   )
>>     (define_insn "*thumb2_addsi_short"
>> -  [(set (match_operand:SI 0 "low_register_operand" "=l,l")
>> -       (plus:SI (match_operand:SI 1 "low_register_operand" "l,0")
>> -                (match_operand:SI 2 "low_reg_or_int_operand" "lPt,Ps")))
>> +  [(set (match_operand:SI 0 "low_register_operand" "=l,l,l")
>> +       (plus:SI (match_operand:SI 1 "low_register_operand" "l,l,0")
>> +                (match_operand:SI 2 "low_reg_or_int_operand" "l,Pt,Ps")))
>>      (clobber (reg:CC CC_REGNUM))]
>>     "TARGET_THUMB2 && reload_completed"
>>     "*
>> @@ -1182,7 +1184,7 @@
>>     "
>>     [(set_attr "predicable" "yes")
>>      (set_attr "length" "2")
>> -   (set_attr "type" "alu_sreg")]
>> +   (set_attr "type" "alu_sreg,alu_imm,alu_imm")]
>>   )
>>     (define_insn "*thumb2_subsi_short"
>> @@ -1226,10 +1228,10 @@
>>   (define_insn "thumb2_addsi3_compare0"
>>     [(set (reg:CC_NOOV CC_REGNUM)
>>         (compare:CC_NOOV
>> -         (plus:SI (match_operand:SI 1 "s_register_operand" "l,  0, r")
>> -                  (match_operand:SI 2 "arm_add_operand"    "lPt,Ps,rIL"))
>> +         (plus:SI (match_operand:SI 1 "s_register_operand" "l,l,  0,r,r")
>> +                  (match_operand:SI 2 "arm_add_operand"
>> "l,Pt,Ps,r,IL"))
>>           (const_int 0)))
>> -   (set (match_operand:SI 0 "s_register_operand" "=l,l,r")
>> +   (set (match_operand:SI 0 "s_register_operand" "=l,l,l,r,r")
>>         (plus:SI (match_dup 1) (match_dup 2)))]
>>     "TARGET_THUMB2"
>>     "*
>> @@ -1246,8 +1248,8 @@
>>         return \"adds\\t%0, %1, %2\";
>>     "
>>     [(set_attr "conds" "set")
>> -   (set_attr "length" "2,2,4")
>> -   (set_attr "type" "alu_sreg")]
>> +   (set_attr "length" "2,2,2,4,4")
>> +   (set_attr "type" "alus_sreg,alus_imm,alus_imm,alus_sreg,alus_imm")]
>>   )
>>
>
>
> In the other patterns in arm.md you didn't split up the alternatives
> but instead used an if_then_else in the set_attr_alternative to
> differentiate
> the case where the operand is constant.
> Any particular reason why you split up the alternatives here?

I think that I tried to be consistent with the implementation of
*thumb2_addsi3_compare0_scratch insn, it is also due to this work was
interrupted several time and I wasn't really consistent with myself ;)

> In my opinion, having fewer alternatives at the expense of a more complex
> definition
> of 'type' is preferable, but someone might have a stronger opinion in the
> other
> direction.

I also prefer fewer alternatives, I'll rework the patch that way and
refactor *thumb2_addsi3_compare0_scratch too (or will do the opposite
if a good argument comes in that thread).

> The patch looks ok to me otherwise, but please respin with the comments
> above addressed.
>
> Thanks for cleaning this up!

Thanks for the review Kyrill

Cheers,
Yvan

> Kyrill
>