Re: [PATCH: PR target/40835] Remove comparison with 0 after instruction movs for thumb

[Richard Earnshaw <rearnsha at arm dot com>]

On Thu, 2009-10-29 at 17:11 +0800, Carrot Wei wrote:
> In thumb mode the cmp instruction in the following sequence can be removed
> since the previous movs instruction has already set the condition code
> according to the value of r1.
> 
>        movs r0, r1
>        cmp  r1, 0
>        beq  .L3
> 
> In thumb mode we don't have insn patterns to express compare and conditional
> branch separately, so this optimization is implemented as a peephole rule.
> 

I don't like peepholes; they usually shouldn't be necessary; though I
accept that it might be hard for combine to spot this case because it
isn't identified by the data-flow (the output of the mov does not flow
into the compare).

However, you are missing several other cases that you could handle
because you've tried to preserve the use of movs rather than using subs:

	subs r0, r1, #0
is identical in flag-setting behaviour to cmp and has the side effect of
setting r0 to r1 as well, so we can replace 

	movs	r0, r1
	cmp	r1, #0
	b<any>	...

with
	subs	r0, r1, #0
	b<any>	...

Please can you at least update the patch to handle this.

R.

> Test:
> This patch was applied to trunk GCC and tested on the arm emulator with newlib.
> No new failure.
> 
> ChangeLog:
> 2009-10-29  Wei Guozhi  <carrot@google.com>
> 
>         PR target/40835
>         * config/arm/arm.c (emit_branch_after_movs): New function.
>         (removable_cmp_0): New function.
>         * config/arm/arm-protos.h (emit_branch_after_movs): Declare it.
>         (removable_cmp_0): Declare it.
>         * config/arm/arm.md: Add peephole rule to do the optimization.
> 		
> 2009-10-29  Wei Guozhi  <carrot@google.com>
> 
>         PR target/40835
>         * gcc.target/arm/pr40835: New testcase.
> 
> 
> thanks
> Wei Guozhi
> 
> 
> Index: arm-protos.h
> ===================================================================
> --- arm-protos.h        (revision 153642)
> +++ arm-protos.h        (working copy)
> @@ -146,7 +146,8 @@ extern const char *vfp_output_fstmd (rtx
>  extern void arm_set_return_address (rtx, rtx);
>  extern int arm_eliminable_register (rtx);
>  extern const char *arm_output_shift(rtx *, int);
> -
> +extern bool removable_cmp_0 (rtx cmp_op);
> +extern const char *emit_branch_after_movs (rtx *operands);
>  extern bool arm_output_addr_const_extra (FILE *, rtx);
> 
>  #if defined TREE_CODE
> 
> 
> Index: arm.c
> ===================================================================
> --- arm.c       (revision 153642)
> +++ arm.c       (working copy)
> @@ -21195,4 +21195,63 @@ arm_have_conditional_execution (void)
>    return !TARGET_THUMB1;
>  }
> 
> +/* In thumb mode the cmp instruction in the following sequence can be
> +   removed since the previous movs instruction has already set the
> +   condition code according to the value of r1.
> +       movs r0, r1
> +       cmp  r1, 0
> +       beq  .L3      */
> +
> +const char *
> +emit_branch_after_movs (rtx *operands)
> +{
> +  char buf[100];
> +
> +  /* operands[2] is the compare rtx.  */
> +  switch (GET_CODE (operands[2]))
> +    {
> +      case EQ:
> +        sprintf (buf, "beq");
> +        break;
> +
> +      case NE:
> +        sprintf (buf, "bne");
> +        break;
> +
> +      case LT:
> +        sprintf (buf, "bmi");
> +        break;
> +
> +      case GE:
> +        sprintf (buf, "bpl");
> +        break;
> +
> +      default:
> +        gcc_unreachable ();
> +    }
> +
> +  output_asm_insn ("movs\t%0, %1", operands);
> +  strcat (buf, "\t%l3");
> +  output_asm_insn (buf, operands);
> +  return "";
> +}
> +
> +/* If the second operand is 0 the following compare rtx codes depend on N
> +   and Z flags only, which have been set by previous movs instruction. So
> +   can be removed.  */
> +bool
> +removable_cmp_0 (rtx cmp_op)
> +{
> +  switch (GET_CODE (cmp_op))
> +    {
> +      case EQ:
> +      case NE:
> +      case LT:
> +      case GE:
> +        return true;
> +      default:
> +        return false;
> +    }
> +}
> +
>  #include "gt-arm.h"
> 
> 
> Index: arm.md
> ===================================================================
> --- arm.md      (revision 153642)
> +++ arm.md      (working copy)
> @@ -7708,6 +7708,28 @@
>                 (const_int 8))))]
>  )
> 
> +;; In thumb mode the cmp instruction in the following sequence can be
> +;; removed since the previous movs instruction has already set the
> +;; condition code according to the value of r1.
> +;;    movs r0, r1
> +;;    cmp  r1, 0
> +;;    beq  .L3
> +
> +(define_peephole
> +  [(set (match_operand:SI 0 "low_register_operand" "")
> +        (match_operand:SI 1 "low_register_operand" ""))
> +   (set (pc)
> +        (if_then_else (match_operator 2 "arm_comparison_operator"
> +                       [(match_dup 1) (const_int 0)])
> +                      (label_ref (match_operand 3 "" ""))
> +                      (pc)))]
> +  "TARGET_THUMB && (get_attr_length (insn) == 4)
> +   && removable_cmp_0 (operands[2])"
> +  "*
> +  return emit_branch_after_movs (operands);
> +  "
> +)
> +
>  ;; Comparison and test insns
> 
>  (define_insn "*arm_cmpsi_insn"
> 
> 
> Index: pr40835.c
> ===================================================================
> --- pr40835.c   (revision 0)
> +++ pr40835.c   (revision 0)
> @@ -0,0 +1,39 @@
> +/* { dg-options "-mthumb -Os -march=armv5te" }  */
> +/* { dg-final { scan-assembler-not "cmp" } } */
> +
> +int bar();
> +void goo(int, int);
> +
> +void eq()
> +{
> +  int v = bar();
> +  if (v == 0)
> +    return;
> +  goo(1, v);
> +}
> +
> +void ge()
> +{
> +  int v = bar();
> +  if (v >= 0)
> +    return;
> +  goo(1, v);
> +}
> +
> +void lt()
> +{
> +  int v = bar();
> +  if (v < 0)
> +    return;
> +  goo(1, v);
> +}
> +
> +unsigned int foo();
> +
> +void leu()
> +{
> +  unsigned int v = foo();
> +  if (v <= 0)
> +    return;
> +  goo(1, v);
> +}