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

On 07/06/10 09:48, Carrot Wei 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.

My apologies for the delay responding to this, I've been pondering Ian's 
comments about the cost of adding an extra scratch register.  We really 
don't want to do that if there isn't one available for free (using CMN 
will be much cheaper).

I think the best way to handle that is the way we handle similar 
conversions to use 16-bit thumb instructions, namely to use peephole2. 
That runs after register allocation and will tell us quickly if we can 
either clobber the input operand, or if there's a suitable scratch 
register elsewhere that's available.

To that effect, I've just committed the attached patch.

R.

2010-06-19  Richard Earnshaw  <rearnsha@arm.com>

	PR target/44072
	* arm.md (cmpsi2_addneg): Prefer emitting adds to subs with a negative
	immediate.
	* constraints.md (Pw, Px): New constraints.
	* thumb2.md (cmpsi2_addneg peephole2): New peepholes.

	PR target/44072
	* gcc.target/arm/thumb2-cmpneg2add-1.c: New test.
	* gcc.target/arm/thumb2-cmpneg2add-2.c: New test.