This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][ARM,ifcvt] Improve use of conditional execution in thumb mode.


On 14/02/12 17:30, Richard Earnshaw wrote:
> On 14/02/12 16:53, Andrew Stubbs wrote:
>> Hi all,
>>
>> I've discovered that GCC does not use ARM conditional execution for
>> 16-bit Thumb opcodes in many cases. It's fine for individual
>> instructions, but if-conversion of basic blocks with more than one
>> instruction fails.
>>
>> E.g.
>>
>> int
>> foo (int a, int b)
>> {
>>    if (a != b)
>>      {
>>        a = a << b;
>>        a = a >> 1;
>>      }
>>
>>    return a + b;
>> }
>>
>> The current compiler gives:
>>
>> foo:
>>          cmp     r0, r1
>>          beq     .L2
>>          lsls    r0, r0, r1
>>          asrs    r0, r0, #1
>> .L2:
>>          adds    r0, r0, r1
>>          bx      lr
>>
>> With my patch I get this:
>>
>> foo:
>>          cmp     r0, r1
>>          itt     ne
>>          lslne   r0, r0, r1
>>          asrne   r0, r0, #1
>>          adds    r0, r0, r1
>>          bx      lr
>>
>> The problem comes from the fact that the compiler prefers "lsls" over
>> "lsl" because the former is a 16-bit encoding, and the latter a 32-bit
>> encoding. There's actually a peephole optimization defined to make this
>> happen wherever the CC register is not live.
>>
>> This is fine in unconditional code, but the CC register clobber means
>> that it's only possible to convert it to conditional code if it is the
>> last instruction in the IT block, so if-conversion fails on the above
>> example.
>>
>> My patch introduces a new target hook "IFCVT_OVERRIDE_MODIFIED_TEST"
>> that allows the CC clobber to be ignored on such instructions, and uses
>> IFCVT_MODIFY_INSN to convert from "lsls" to "lsl<c>" where possible.
>>
>> I've also introduced a new instruction attribute "it_cc" to indicate
>> which instruction patterns are affected.
>>
>> OK for trunk, once stage 1 reopens?
>>
>> Andrew
>>
> 
> Bernds checked in a patch last year (or maybe even before that) to make
> the selection of flags clobbered insns run very late (certainly after
> condexec had run), so I'm not sure why you're not seeing this.
> 

Hm, you mentioned some peepholes.  Try removing them....

R.

> R.
> 
>>
>> ifcvt_modify_insn.patch
>>
>>
>> 2012-02-14  Andrew Stubbs  <ams@codesourcery.com>
>>
>>       gcc/
>>       * config/arm/arm-protos.h (arm_ifcvt_modify_insn): New prototype.
>>       (arm_ifcvt_override_modified_test): New prototype.
>>       * config/arm/arm.c (thumb_insn_suitable_for_ifcvt): New function.
>>       (arm_ifcvt_override_modified_test): New function.
>>       (arm_ifcvt_modify_insn): New function.
>>       * config/arm/arm.h (IFCVT_OVERRIDE_MODIFIED_TEST): New macro.
>>       (IFCVT_MODIFY_INSN): New macro.
>>       * config/arm/thumb2.md (it_cc): New attribute.
>>       (thumb2_alusi3_short): Set it_cc attribute.
>>       (thumb2_shiftsi3_short, thumb2_mov<mode>_shortim): Likewise.
>>       (thumb2_addsi_short, thumb2_subsi_short): Likewise.
>>       (thumb2_mulsi_short, thumb2_one_cmplsi2_short): Likewise.
>>       (thumb2_negsi2_short): Likewise.
>>       * doc/tm.texi: Regenerate.
>>       * doc/tm.texi.in (IFCVT_OVERRIDE_MODIFIED_TEST): Document.
>>       * ifcvt.c (cond_exec_process_insns): Add IFCVT_OVERRIDE_MODIFIED_TEST.
>>
>>       gcc/testsuite/
>>       * gcc.target/arm/thumb-ifcvt.c: New test case.
>>
>> ---
>>  gcc/config/arm/arm-protos.h                |    5 ++
>>  gcc/config/arm/arm.c                       |   67 ++++++++++++++++++++++++++++
>>  gcc/config/arm/arm.h                       |    5 ++
>>  gcc/config/arm/thumb2.md                   |   11 +++++
>>  gcc/doc/tm.texi                            |   13 +++++
>>  gcc/doc/tm.texi.in                         |   13 +++++
>>  gcc/ifcvt.c                                |   14 +++++-
>>  gcc/testsuite/gcc.target/arm/thumb-ifcvt.c |   19 ++++++++
>>  8 files changed, 146 insertions(+), 1 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.target/arm/thumb-ifcvt.c
>>
>> diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
>> index 296550a..67396f0 100644
>> --- a/gcc/config/arm/arm-protos.h
>> +++ b/gcc/config/arm/arm-protos.h
>> @@ -244,4 +244,9 @@ extern const struct tune_params *current_tune;
>>  extern int vfp3_const_double_for_fract_bits (rtx);
>>  #endif /* RTX_CODE */
>>
>> +#ifdef BB_HEAD
>> +extern int arm_ifcvt_override_modified_test (rtx, rtx);
>> +extern rtx arm_ifcvt_modify_insn (ce_if_block_t *, rtx, rtx);
>> +#endif
>> +
>>  #endif /* ! GCC_ARM_PROTOS_H */
>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>> index 0bded8d..803e1c9 100644
>> --- a/gcc/config/arm/arm.c
>> +++ b/gcc/config/arm/arm.c
>> @@ -25139,5 +25139,72 @@ vfp3_const_double_for_fract_bits (rtx operand)
>>    return 0;
>>  }
>>
>> +/* Find the portion of INSN that is suitable for if-conversion.
>> +
>> +   Some Thumb mode 16-bit instructions have two variants: one that is
>> +   unconditional but clobbers the condition flags, and one that is
>> +   conditional (in an IT block) and does not clobber anything.
>> +
>> +   There are 32-bit variants that are unconditional but don't clobber
>> +   anything, so the peephole2 pass adds the clobber in order to use the
>> +   smaller instruction encoding.  Unfortunately this defeats the
>> +   if-conversion pass since CC must not be modified in an IT block.
>> +
>> +   The peephole can be reversed, and the instruction converted to
>> +   conditional execution without affecting the size optimization.
>> +
>> +   This function detects these instructions and returns the sub-expression
>> +   that contains the operation, without the clobber.  */
>> +
>> +static rtx
>> +thumb_insn_suitable_for_ifcvt (rtx pattern, rtx insn)
>> +{
>> +  if (GET_CODE (pattern) == PARALLEL
>> +      && XVECLEN (pattern, 0) == 2)
>> +    {
>> +      rtx op = XVECEXP (pattern, 0, 0);
>> +      rtx clobber = XVECEXP (pattern, 0, 1);
>> +
>> +      if (GET_CODE (clobber) == CLOBBER
>> +       && GET_CODE (XEXP (clobber, 0)) == REG
>> +       && REGNO (XEXP (clobber, 0)) == CC_REGNUM
>> +       && get_attr_it_cc (insn) == IT_CC_NOCLOBBER)
>> +     return op;
>> +    }
>> +
>> +  return NULL_RTX;
>> +}
>> +
>> +/* Prevent if-conversion thinking that certain Thumb1 instructions
>> +   clobber CC.  These instruction in fact do not when in an IT block.  */
>> +int
>> +arm_ifcvt_override_modified_test (rtx test, rtx insn)
>> +{
>> +  if (thumb_insn_suitable_for_ifcvt (PATTERN (insn), insn) != NULL_RTX)
>> +    return 0; /* Force *not* modified.  */
>> +
>> +  return -1;
>> +}
>> +
>> +/* Modify insns to enable conditional execution.
>> +
>> +   This function removes CC clobers that impede if-conversion.  See the
>> +   comments on thumb_insn_suitable_for_ifcvt.  */
>> +rtx
>> +arm_ifcvt_modify_insn (ce_if_block_t *ce, rtx pattern, rtx insn)
>> +{
>> +  rtx op, cond;
>> +
>> +  gcc_assert (GET_CODE (pattern) == COND_EXEC);
>> +
>> +  cond = XEXP (pattern, 0);
>> +  op = thumb_insn_suitable_for_ifcvt (XEXP (pattern, 1), insn);
>> +
>> +  if (op != NULL_RTX)
>> +    return gen_rtx_COND_EXEC (VOIDmode, cond, op);
>> +
>> +  return pattern;
>> +}
>> +
>>  #include "gt-arm.h"
>>
>> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
>> index 5a78125..0b7f332 100644
>> --- a/gcc/config/arm/arm.h
>> +++ b/gcc/config/arm/arm.h
>> @@ -2220,4 +2220,9 @@ extern const char *host_detect_local_cpu (int argc, const char **argv);
>>
>>  #define DRIVER_SELF_SPECS MCPU_MTUNE_NATIVE_SPECS
>>
>> +#define IFCVT_OVERRIDE_MODIFIED_TEST(TEST,INSN) \
>> +  arm_ifcvt_override_modified_test ((TEST), (INSN))
>> +#define IFCVT_MODIFY_INSN(CE,PATTERN,INSN) \
>> +  (PATTERN) = arm_ifcvt_modify_insn ((CE), (PATTERN), (INSN))
>> +
>>  #endif /* ! GCC_ARM_H */
>> diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md
>> index 05585da..454c42f 100644
>> --- a/gcc/config/arm/thumb2.md
>> +++ b/gcc/config/arm/thumb2.md
>> @@ -24,6 +24,9 @@
>>  ;; changes made in armv5t as "thumb2".  These are considered part
>>  ;; the 16-bit Thumb-1 instruction set.
>>
>> +;; Does an insn clobber CC if it appears in an IT block.  */
>> +(define_attr "it_cc" "undef,clobber,noclobber" (const_string "undef"))
>> +
>>  (define_insn "*thumb2_incscc"
>>    [(set (match_operand:SI 0 "s_register_operand" "=r,r")
>>          (plus:SI (match_operator:SI 2 "arm_comparison_operator"
>> @@ -674,6 +677,7 @@
>>     && GET_CODE(operands[3]) != MINUS"
>>    "%I3%!\\t%0, %1, %2"
>>    [(set_attr "predicable" "yes")
>> +   (set_attr "it_cc" "noclobber")
>>     (set_attr "length" "2")]
>>  )
>>
>> @@ -708,6 +712,7 @@
>>         || REG_P(operands[2]))"
>>    "* return arm_output_shift(operands, 2);"
>>    [(set_attr "predicable" "yes")
>> +   (set_attr "it_cc" "noclobber")
>>     (set_attr "shift" "1")
>>     (set_attr "length" "2")
>>     (set (attr "type") (if_then_else (match_operand 2 "const_int_operand" "")
>> @@ -736,6 +741,7 @@
>>    "TARGET_THUMB2 && reload_completed"
>>    "mov%!\t%0, %1"
>>    [(set_attr "predicable" "yes")
>> +   (set_attr "it_cc" "noclobber")
>>     (set_attr "length" "2")]
>>  )
>>
>> @@ -778,6 +784,7 @@
>>        return \"add%!\\t%0, %1, %2\";
>>    "
>>    [(set_attr "predicable" "yes")
>> +   (set_attr "it_cc" "noclobber")
>>     (set_attr "length" "2")]
>>  )
>>
>> @@ -789,6 +796,7 @@
>>    "TARGET_THUMB2 && reload_completed"
>>    "sub%!\\t%0, %1, %2"
>>    [(set_attr "predicable" "yes")
>> +   (set_attr "it_cc" "noclobber")
>>     (set_attr "length" "2")]
>>  )
>>
>> @@ -905,6 +913,7 @@
>>    "TARGET_THUMB2 && optimize_size && reload_completed"
>>    "mul%!\\t%0, %2, %0"
>>    [(set_attr "predicable" "yes")
>> +   (set_attr "it_cc" "noclobber")
>>     (set_attr "length" "2")
>>     (set_attr "insn" "muls")])
>>
>> @@ -999,6 +1008,7 @@
>>    "TARGET_THUMB2 && reload_completed"
>>    "mvn%!\t%0, %1"
>>    [(set_attr "predicable" "yes")
>> +   (set_attr "it_cc" "noclobber")
>>     (set_attr "length" "2")]
>>  )
>>
>> @@ -1022,6 +1032,7 @@
>>    "TARGET_THUMB2 && reload_completed"
>>    "neg%!\t%0, %1"
>>    [(set_attr "predicable" "yes")
>> +   (set_attr "it_cc" "noclobber")
>>     (set_attr "length" "2")]
>>  )
>>
>> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
>> index 6d41cee..7d09ddb 100644
>> --- a/gcc/doc/tm.texi
>> +++ b/gcc/doc/tm.texi
>> @@ -10879,6 +10879,19 @@ if-statements into conditions combined by @code{and} and @code{or} operations.
>>  being processed and about to be turned into a condition.
>>  @end defmac
>>
>> +@defmac IFCVT_OVERRIDE_MODIFIED_TEST (@var{test}, @var{insn})
>> +Used if the target has instructions that modify the condition
>> +register when executed unconditionally, but do not do so when executed
>> +conditionally, or vice-versa.  @var{TEST} contains the condition test, and
>> +@var{INSN} contains the original insn to be conditionalized.  The macro
>> +should return minus-one if there is to be no override, zero to disregard
>> +an apparent modified condition register, or any other value to indicate
>> +that the instruction does modify the condition register, even if it does
>> +not appear to.  Additionally, if conditionalizing the instruction does
>> +change its behaviour, @code{IFCVT_MODIFY_INSN} should make the appropriate
>> +adjustments to match.
>> +@end defmac
>> +
>>  @defmac IFCVT_MODIFY_INSN (@var{ce_info}, @var{pattern}, @var{insn})
>>  A C expression to modify the @var{PATTERN} of an @var{INSN} that is to
>>  be converted to conditional execution format.  @var{ce_info} points to
>> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
>> index 396f244..52d9e96 100644
>> --- a/gcc/doc/tm.texi.in
>> +++ b/gcc/doc/tm.texi.in
>> @@ -10759,6 +10759,19 @@ if-statements into conditions combined by @code{and} and @code{or} operations.
>>  being processed and about to be turned into a condition.
>>  @end defmac
>>
>> +@defmac IFCVT_OVERRIDE_MODIFIED_TEST (@var{test}, @var{insn})
>> +Used if the target has instructions that modify the condition
>> +register when executed unconditionally, but do not do so when executed
>> +conditionally, or vice-versa.  @var{TEST} contains the condition test, and
>> +@var{INSN} contains the original insn to be conditionalized.  The macro
>> +should return minus-one if there is to be no override, zero to disregard
>> +an apparent modified condition register, or any other value to indicate
>> +that the instruction does modify the condition register, even if it does
>> +not appear to.  Additionally, if conditionalizing the instruction does
>> +change its behaviour, @code{IFCVT_MODIFY_INSN} should make the appropriate
>> +adjustments to match.
>> +@end defmac
>> +
>>  @defmac IFCVT_MODIFY_INSN (@var{ce_info}, @var{pattern}, @var{insn})
>>  A C expression to modify the @var{PATTERN} of an @var{INSN} that is to
>>  be converted to conditional execution format.  @var{ce_info} points to
>> diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
>> index ce60ce2..ff620da 100644
>> --- a/gcc/ifcvt.c
>> +++ b/gcc/ifcvt.c
>> @@ -310,6 +310,7 @@ cond_exec_process_insns (ce_if_block_t *ce_info ATTRIBUTE_UNUSED,
>>    rtx insn;
>>    rtx xtest;
>>    rtx pattern;
>> +  int override_modified = -1;
>>
>>    if (!start || !end)
>>      return FALSE;
>> @@ -338,7 +339,18 @@ cond_exec_process_insns (ce_if_block_t *ce_info ATTRIBUTE_UNUSED,
>>        if (must_be_last)
>>       return FALSE;
>>
>> -      if (modified_in_p (test, insn))
>> +      /* Some instructions modify CC flags when executed unconditionally
>> +      but not when executed conditionally.
>> +         Return values:
>> +         -1 = no override
>> +         0 = force false
>> +         other = force true.  */
>> +#ifdef IFCVT_OVERRIDE_MODIFIED_TEST
>> +      override_modified = IFCVT_OVERRIDE_MODIFIED_TEST (test, insn);
>> +#endif
>> +
>> +      if ((override_modified == -1 && modified_in_p (test, insn))
>> +       || override_modified)
>>       {
>>         if (!mod_ok)
>>           return FALSE;
>> diff --git a/gcc/testsuite/gcc.target/arm/thumb-ifcvt.c b/gcc/testsuite/gcc.target/arm/thumb-ifcvt.c
>> new file mode 100644
>> index 0000000..b03bbce
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/thumb-ifcvt.c
>> @@ -0,0 +1,19 @@
>> +/* Check that Thumb 16-bit shifts can be if-converted.  */
>> +/* { dg-do compile } */
>> +/* { dg-require-effective-target arm_thumb2_ok } */
>> +/* { dg-options "-O2" } */
>> +
>> +int
>> +foo (int a, int b)
>> +{
>> +  if (a != b)
>> +    {
>> +      a = a << b;
>> +      a = a >> 1;
>> +    }
>> +
>> +  return a + b;
>> +}
>> +
>> +/* { dg-final { scan-assembler "lslne" } } */
>> +/* { dg-final { scan-assembler "asrne" } } */
> 
> 
> 



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]