[PATCH 2/5][Arm] New pattern for CSINV instructions

Kyrylo Tkachov Kyrylo.Tkachov@arm.com
Thu Aug 6 09:30:42 GMT 2020


Hi Omar,

> -----Original Message-----
> From: Omar Tahir <Omar.Tahir@arm.com>
> Sent: 05 August 2020 12:42
> To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>; nickc@redhat.com;
> Ramana Radhakrishnan <Ramana.Radhakrishnan@arm.com>; Richard
> Earnshaw <Richard.Earnshaw@arm.com>; gcc-patches@gcc.gnu.org
> Subject: RE: [PATCH 2/5][Arm] New pattern for CSINV instructions
> 
> Hi Kyrill,
> 
> > -/* Only thumb1 can't support conditional execution, so return true if
> > -   the target is not thumb1.  */
> > static bool
> >
> >
> > Functions should have comments in GCC. Can you please write something
> describing the new logic of the function.
> >
> > arm_have_conditional_execution (void)
> > {
> > -  return !TARGET_THUMB1;
> > +  bool has_cond_exec, enable_ifcvt_trans;
> > +
> > +  /* Only THUMB1 cannot support conditional execution. */
> > +  has_cond_exec = !TARGET_THUMB1;
> > +
> > +  /* When TARGET_COND_ARITH is defined we'd like to turn on some ifcvt
> > +     transformations before reload. */
> > +  enable_ifcvt_trans = TARGET_COND_ARITH && !reload_completed;
> > +
> > +  /* The ifcvt transformations are only turned on if we return false. */
> > +  return has_cond_exec && !enable_ifcvt_trans;
> >
> > I don't think that comment is very useful. Perhaps "Enable ifcvt
> transformations only if..."
> >
> > }
> 
> Fixed, let me know if the new comments are a bit clearer now.
> 
> > +(define_constraint "Z"
> > +  "@internal
> > +   Integer constant zero."
> > +  (match_test "op == const0_rtx"))
> >
> >
> > We're usually wary of adding more constraints unless necessary as it gets
> complicated to read patterns quickly (especially once we get into multi-letter
> constraints).
> > I think you can reuse the existing "Pz" constraint for your purposes.
> 
> Yes Pz works, I'll replace Z with Pz in the other patches as well. In patch 5 I
> introduce UM (-1) and U1 (1), I don't think there's any existing combination
> of constraints that can be used instead.

Great!

> 
> >
> > Ok with those changes.
> > If you'd like to commit it yourself please apply for write access at
> https://sourceware.org/cgi-bin/pdw/ps_form.cgi listing my email address
> from MAINTAINERS as the approver.
> 
> Excellent, thanks. If the other three patches are okay I'll commit them as well?

Please wait for review before committing them, but once they're ok'ed feel free to push them (please make sure proper testing is done so that trunk is not left in a broken state).

Thanks,
Kyrill

> 
> Thanks,
> Omar
> 
> ---
> 
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index dac9a6fb5c4..e1bb2db9c8a 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -29833,12 +29833,23 @@ arm_frame_pointer_required (void)
>    return false;
>  }
> 
> -/* Only thumb1 can't support conditional execution, so return true if
> -   the target is not thumb1.  */
> +/* Implement the TARGET_HAVE_CONDITIONAL_EXECUTION hook.
> +   All modes except THUMB1 have conditional execution.
> +   If we have conditional arithmetic, return false before reload to
> +   enable some ifcvt transformations. */
>  static bool
>  arm_have_conditional_execution (void)
>  {
> -  return !TARGET_THUMB1;
> +  bool has_cond_exec, enable_ifcvt_trans;
> +
> +  /* Only THUMB1 cannot support conditional execution. */
> +  has_cond_exec = !TARGET_THUMB1;
> +
> +  /* Enable ifcvt transformations if we have conditional arithmetic, but only
> +     before reload. */
> +  enable_ifcvt_trans = TARGET_COND_ARITH && !reload_completed;
> +
> +  return has_cond_exec && !enable_ifcvt_trans;
>  }
> 
>  /* The AAPCS sets the maximum alignment of a vector to 64 bits.  */
> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
> index 30e1d6dc994..d67c91796e4 100644
> --- a/gcc/config/arm/arm.h
> +++ b/gcc/config/arm/arm.h
> @@ -177,6 +177,10 @@ emission of floating point pcs attributes.  */
> 
>  #define TARGET_CRC32			(arm_arch_crc)
> 
> +/* Thumb-2 but also has some conditional arithmetic instructions like csinc,
> +   csinv, etc. */
> +#define TARGET_COND_ARITH		(arm_arch8_1m_main)
> +
>  /* The following two macros concern the ability to execute coprocessor
>     instructions for VFPv3 or NEON.  TARGET_VFP3/TARGET_VFPD32 are
> currently
>     only ever tested when we know we are generating for VFP hardware; we
> need
> diff --git a/gcc/config/arm/predicates.md b/gcc/config/arm/predicates.md
> index 981eec520ba..2144520829c 100644
> --- a/gcc/config/arm/predicates.md
> +++ b/gcc/config/arm/predicates.md
> @@ -485,6 +485,18 @@
>    (and (match_operand 0 "expandable_comparison_operator")
>         (match_test "maybe_get_arm_condition_code (op) != ARM_NV")))
> 
> +(define_special_predicate "arm_comparison_operation"
> +  (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,unordered,
> +         ordered,unlt,unle,unge,ungt")
> +{
> +  if (XEXP (op, 1) != const0_rtx)
> +    return false;
> +  rtx op0 = XEXP (op, 0);
> +  if (!REG_P (op0) || REGNO (op0) != CC_REGNUM)
> +    return false;
> +  return maybe_get_arm_condition_code (op) != ARM_NV;
> +})
> +
>  (define_special_predicate "lt_ge_comparison_operator"
>    (match_code "lt,ge"))
> 
> diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md
> index 793f6706868..ecc903970db 100644
> --- a/gcc/config/arm/thumb2.md
> +++ b/gcc/config/arm/thumb2.md
> @@ -938,6 +938,20 @@
>     (set_attr "type" "multiple")]
>  )
> 
> +(define_insn "*thumb2_csinv"
> +  [(set (match_operand:SI 0 "arm_general_register_operand" "=r, r")
> +  (if_then_else:SI
> +    (match_operand 1 "arm_comparison_operation" "")
> +    (not:SI (match_operand:SI 2 "arm_general_register_operand" "r, r"))
> +    (match_operand:SI 3 "reg_or_zero_operand" "r, Pz")))]
> +  "TARGET_COND_ARITH"
> +  "@
> +   csinv\\t%0, %3, %2, %D1
> +   csinv\\t%0, zr, %2, %D1"
> +  [(set_attr "type" "csel")
> +   (set_attr "predicable" "no")]
> +)
> +
>  (define_insn "*thumb2_movcond"
>    [(set (match_operand:SI 0 "s_register_operand" "=Ts,Ts,Ts")
>  	(if_then_else:SI
> @@ -947,7 +961,7 @@
>  	 (match_operand:SI 1 "arm_rhs_operand" "0,TsI,?TsI")
>  	 (match_operand:SI 2 "arm_rhs_operand" "TsI,0,TsI")))
>     (clobber (reg:CC CC_REGNUM))]
> -  "TARGET_THUMB2"
> +  "TARGET_THUMB2 && !TARGET_COND_ARITH"
>    "*
>    if (GET_CODE (operands[5]) == LT
>        && (operands[4] == const0_rtx))
> diff --git a/gcc/testsuite/gcc.target/arm/csinv-1.c
> b/gcc/testsuite/gcc.target/arm/csinv-1.c
> new file mode 100644
> index 00000000000..6b5383aa913
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/csinv-1.c
> @@ -0,0 +1,23 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target arm_arch_v8_1m_main_ok } */
> +/* { dg-options "-O2 -march=armv8.1-m.main" } */
> +
> +int
> +test_csinv32_condasn1(int w0, int w1, int w2, int w3)
> +{
> +  int w4;
> +
> +  /* { dg-final { scan-assembler "csinv\tr\[0-9\]*.*ne" } } */
> +  w4 = (w0 == w1) ? ~w2 : w3;
> +  return w4;
> +}
> +
> +int
> +test_csinv32_condasn2(int w0, int w1, int w2, int w3)
> +{
> +  int w4;
> +
> +  /* { dg-final { scan-assembler "csinv\tr\[0-9\]*.*eq" } } */
> +  w4 = (w0 == w1) ? w3 : ~w2;
> +  return w4;
> +}


More information about the Gcc-patches mailing list