[PING] [PATCH: PR target/44072] Replace "cmp r0, -1" by "adds r0, 1" in thumb2

[Carrot Wei <carrot at google dot com>]

On Thu, Jun 10, 2010 at 2:30 PM, Carrot Wei <carrot@google.com> wrote:
> ping
>
> On Mon, Jun 7, 2010 at 4:48 PM, Carrot Wei <carrot@google.com> wrote:
>> Hi
>>
>> I've modified the patch a little. Please take another look.
>>
>> 1. Add code to clobber cc register.
>> 2. Change the constraint "l,?h,0" to "l,0,h".
>>
>> It should be noted that the change of constraint doesn't bring the expected
>> result. For the following simple code:
>>
>> if (x == -8)
>> ? ?foo1();
>>
>> GCC generates:
>>
>> ? ? ? mov ? ? ip, r0
>> ? ? ? cmp ? ? ip, #-8
>> ? ? ? bne ? ? .L1
>> ? ? ? ...
>>
>> It is even worse than current default behavior. Ian explained as:
>>
>> "At first glance, I would say that the third alternative does not
>> require a scratch register whereas the second alternative does require
>> one. ?Therefore, the second alternative costs more. ?Scratch registers
>> are, rightly, expensive, because in the general case (though not in
>> this tiny case) they require spilling values onto the stack."
>>
>> So gcc can't correctly model the costs of the second and third alternatives
>> in a situation without high register pressure. I will open a bug entry after
>> check in this patch.
>>
>> In practice it is not a big problem. Because -1 is the most frequently compared
>> negative constant since it is usually used to indicate an error return value.
>> Testing with CSiBE confirms this. There are only less than 10 functions get
>> larger but more than 10 times number of functions get smaller. This patch also
>> passed regression testing on qemu.
>>
>> thanks
>> Guozhi
>>
>>
>> ChangeLog:
>> 2010-06-07 ?Wei Guozhi ?<carrot@google.com>
>>
>> ? ? ? ?PR target/44072
>> ? ? ? ?* config/arm/thumb2.md (thumb2_cbranchsi4_scratch): New insn pattern.
>>
>> ChangeLog:
>> 2010-06-07 ?Wei Guozhi ?<carrot@google.com>
>>
>> ? ? ? ?PR target/44072
>> ? ? ? ?* gcc.target/arm/pr44072.c: New testcase.
>>
>>
>> Index: thumb2.md
>> ===================================================================
>> --- thumb2.md ? (revision 160356)
>> +++ thumb2.md ? (working copy)
>> @@ -1591,3 +1591,75 @@
>> ? ? ? ? ? ? ? ?(const_int 8)
>> ? ? ? ? ? ? ? ?(const_int 10)))))]
>> ?)
>> +
>> +(define_insn "thumb2_cbranchsi4_scratch"
>> + ?[(set (pc) (if_then_else
>> + ? ? ? ? ? ? (match_operator 4 "arm_comparison_operator"
>> + ? ? ? ? ? ? ?[(match_operand:SI 1 "s_register_operand" "l,0,h")
>> + ? ? ? ? ? ? ? (match_operand:SI 2 "thumb1_cmpneg_operand" "Pt,Ps,Ps")])
>> + ? ? ? ? ? ? (label_ref (match_operand 3 "" ""))
>> + ? ? ? ? ? ? (pc)))
>> + ? (clobber (match_scratch:SI 0 "=l,l,X"))
>> + ? (clobber (reg:CC CC_REGNUM))]
>> + ?"TARGET_THUMB2"
>> + ?"*
>> + ?if (which_alternative == 2)
>> + ? ?{
>> + ? ? ?output_asm_insn (\"cmp\\t%1, %2\", operands);
>> + ? ? ?switch (get_attr_length (insn))
>> + ? ? ? {
>> + ? ? ? ? case 6: ?return \"b%d4\\t%l3\";
>> + ? ? ? ? case 8: ?return \"b%D4\\t.LCB%=\;b\\t%l3\\t%@long jump\\n.LCB%=:\";
>> + ? ? ? ? default: return \"b%D4\\t.LCB%=\;bl\\t%l3\\t%@far jump\\n.LCB%=:\";
>> + ? ? ? }
>> + ? ?}
>> + ?else
>> + ? ?{
>> + ? ? ?if (which_alternative == 0)
>> + ? ? ? output_asm_insn (\"adds\\t%0, %1, #%n2\", operands);
>> + ? ? ?else
>> + ? ? ? output_asm_insn (\"adds\\t%0, #%n2\", operands);
>> +
>> + ? ? ?switch (get_attr_length (insn))
>> + ? ? ? {
>> + ? ? ? ? case 4: ?return \"b%d4\\t%l3\";
>> + ? ? ? ? case 6: ?return \"b%D4\\t.LCB%=\;b\\t%l3\\t%@long jump\\n.LCB%=:\";
>> + ? ? ? ? default: return \"b%D4\\t.LCB%=\;bl\\t%l3\\t%@far jump\\n.LCB%=:\";
>> + ? ? ? }
>> + ? ?}
>> + ?"
>> + ?[(set (attr "far_jump")
>> + ? ? ? (if_then_else
>> + ? ? ? ? (eq (symbol_ref ("which_alternative"))
>> + ? ? ? ? ? ? ? ? ? ? ? ? (const_int 2))
>> + ? ? ? ? (if_then_else
>> + ? ? ? ? ? (eq_attr "length" "10")
>> + ? ? ? ? ? (const_string "yes")
>> + ? ? ? ? ? (const_string "no"))
>> + ? ? ? ? (if_then_else
>> + ? ? ? ? ? (eq_attr "length" "8")
>> + ? ? ? ? ? (const_string "yes")
>> + ? ? ? ? ? (const_string "no"))))
>> + ? (set (attr "length")
>> + ? ? ? (if_then_else
>> + ? ? ? ? (eq (symbol_ref ("which_alternative"))
>> + ? ? ? ? ? ? ? ? ? ? ? ? (const_int 2))
>> + ? ? ? ? (if_then_else
>> + ? ? ? ? ? (and (ge (minus (match_dup 3) (pc)) (const_int -250))
>> + ? ? ? ? ? ? ? ?(le (minus (match_dup 3) (pc)) (const_int 256)))
>> + ? ? ? ? ? (const_int 6)
>> + ? ? ? ? ? (if_then_else
>> + ? ? ? ? ? ? ? (and (ge (minus (match_dup 3) (pc)) (const_int -2040))
>> + ? ? ? ? ? ? ? ? ? ?(le (minus (match_dup 3) (pc)) (const_int 2048)))
>> + ? ? ? ? ? ? ? (const_int 8)
>> + ? ? ? ? ? ? ? (const_int 10)))
>> + ? ? ? ? (if_then_else
>> + ? ? ? ? ? (and (ge (minus (match_dup 3) (pc)) (const_int -250))
>> + ? ? ? ? ? ? ? ?(le (minus (match_dup 3) (pc)) (const_int 256)))
>> + ? ? ? ? ? (const_int 4)
>> + ? ? ? ? ? (if_then_else
>> + ? ? ? ? ? ? ? (and (ge (minus (match_dup 3) (pc)) (const_int -2040))
>> + ? ? ? ? ? ? ? ? ? ?(le (minus (match_dup 3) (pc)) (const_int 2048)))
>> + ? ? ? ? ? ? ? (const_int 6)
>> + ? ? ? ? ? ? ? (const_int 8)))))]
>> +)
>>
>> Index: pr44072.c
>> ===================================================================
>> --- pr44072.c ? (revision 0)
>> +++ pr44072.c ? (revision 0)
>> @@ -0,0 +1,9 @@
>> +/* { dg-options "-march=armv7-a -mthumb -Os" } ?*/
>> +/* { dg-final { scan-assembler "adds" } } */
>> +
>> +void foo1();
>> +void bar5(int x)
>> +{
>> + ?if (x == -1)
>> + ? ?foo1();
>> +}
>>
>>
>> On Thu, May 13, 2010 at 3:28 PM, Richard Earnshaw <rearnsha@arm.com> wrote:
>>> On Wed, 2010-05-12 at 16:43 +0800, Carrot Wei wrote:
>>>> Hi
>>>>
>>>> Both "cmp r0, -1" and "adds r0, 1" have the same effect when used to set
>>>> conditions for following branch. But adds with small positive immediate is a
>>>> 16 bit instruction and cmp with negative immediate is a 32 bit instruction. So
>>>> we prefer adds in thumb2.
>>>>
>>>> Thumb1 already has an insn pattern "cbranchsi4_scratch" to do this, this patch
>>>> adds the corresponding insn pattern for thumb2.
>>>>
>>>> +
>>>> +(define_insn "thumb2_cbranchsi4_scratch"
>>>> + ?[(set (pc) (if_then_else
>>>> + ? ? ? ? ? ? (match_operator 4 "arm_comparison_operator"
>>>> + ? ? ? ? ? ? ?[(match_operand:SI 1 "s_register_operand" "l,?h,0")
>>>> + ? ? ? ? ? ? ? (match_operand:SI 2 "thumb1_cmpneg_operand" "Pt,Ps,Ps")])
>>>
>>> You should put the constrains in the order that they are preferred,
>>> rather than trying to force the behaviour with '?'. ?So the constraints
>>> for op1 should be "l,0,h" and the rest of the pattern adjusted to match.
>>>
>>> OK with that change.
>>>
>>> R.
>>>
>>>
>>
>