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

[Carrot Wei <carrot at google dot com>]

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.
>
>