Hi Omar,
> -----Original Message-----
> From: Omar Tahir <omar.ta...@arm.com>
> Sent: 05 August 2020 12:42
> To: Kyrylo Tkachov <kyrylo.tkac...@arm.com>; ni...@redhat.com;
> Ramana Radhakrishnan <ramana.radhakrish...@arm.com>; Richard
> Earnshaw <richard.earns...@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;
> +}
