RE: [PATCH 2/4 v2][AArch64] Add support for FCCMP

[Wilco Dijkstra <Wilco dot Dijkstra at arm dot com>]

Adding Bernd - would you mind reviewing the ccmp.c change please?

> -----Original Message-----
> From: James Greenhalgh [mailto:james.greenhalgh@arm.com]
> Sent: 15 December 2015 16:42
> To: Wilco Dijkstra
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH 2/4 v2][AArch64] Add support for FCCMP
> 
> On Tue, Dec 15, 2015 at 10:32:50AM +0000, Wilco Dijkstra wrote:
> > ping
> >
> > > -----Original Message-----
> > > From: Wilco Dijkstra [mailto:Wilco.Dijkstra@arm.com]
> > > Sent: 17 November 2015 18:36
> > > To: gcc-patches@gcc.gnu.org
> > > Subject: [PATCH 2/4 v2][AArch64] Add support for FCCMP
> > >
> > > (v2 version removes 4 enums)
> > >
> > > This patch adds support for FCCMP. This is trivial with the new CCMP representation - remove the restriction of FP in ccmp.c and
> add
> > > FCCMP patterns. Add a test to ensure FCCMP/FCCMPE are emitted as expected.
> > >
> > > OK for commit?
> 
> The AArch64 code-generation parts of this are OK, though please wait for
> an OK on the ccmp.c changes before committing, and please revisit the
> testcase.
> 
> Sorry that this took a long time to get to.

No problem.

> > > ChangeLog:
> > > 2015-11-18  Wilco Dijkstra  <wdijkstr@arm.com>
> > >
> > > 	* gcc/ccmp.c (ccmp_candidate_p): Remove integer-only restriction.
> 
> Drop the gcc/ from the paths here and below.
> 
> > > 	* gcc/config/aarch64/aarch64.md (fccmp<mode>): New pattern.
> > > 	(fccmpe<mode>): Likewise.
> > > 	(fcmp): Rename to fcmp and globalize pattern.
> > > 	(fcmpe): Likewise.
> > > 	* gcc/config/aarch64/aarch64.c (aarch64_gen_ccmp_first): Add FP support.
> > > 	(aarch64_gen_ccmp_next): Add FP support.
> > >
> > > gcc/testsuite/
> > > 	* gcc.target/aarch64/ccmp_1.c: New testcase.
> 
> This testcase doesn't look very helpful to me. You only end up checking if
> *any* of the tests compile to fccmp/fccmpe rather than *all* the tests. Should
> this use a scan-assembler-times directive to count the number of times a
> particular instruction appears?

There are no costs involved so there is no guarantee which CCMPs we will see
generated. After patch 3 and 4, the order is better defined and the testcase is
updated to reflect what we expect to be generated.

The alternative would be to not add the testcase here, but in part 4. However in
internal review it was requested to add it to this part of the patch...

Wilco

> > > ---
> > >  gcc/ccmp.c                                |  6 ---
> > >  gcc/config/aarch64/aarch64.c              | 24 +++++++++
> > >  gcc/config/aarch64/aarch64.md             | 34 ++++++++++++-
> > >  gcc/testsuite/gcc.target/aarch64/ccmp_1.c | 84 +++++++++++++++++++++++++++++++
> > >  4 files changed, 140 insertions(+), 8 deletions(-)
> > >  create mode 100644 gcc/testsuite/gcc.target/aarch64/ccmp_1.c
> > >
> > > diff --git a/gcc/ccmp.c b/gcc/ccmp.c
> > > index 58ac126..3698a7d 100644
> > > --- a/gcc/ccmp.c
> > > +++ b/gcc/ccmp.c
> > > @@ -112,12 +112,6 @@ ccmp_candidate_p (gimple *g)
> > >        || gimple_bb (gs0) != gimple_bb (g))
> > >      return false;
> > >
> > > -  if (!(INTEGRAL_TYPE_P (TREE_TYPE (gimple_assign_rhs1 (gs0)))
> > > -       || POINTER_TYPE_P (TREE_TYPE (gimple_assign_rhs1 (gs0))))
> > > -      || !(INTEGRAL_TYPE_P (TREE_TYPE (gimple_assign_rhs1 (gs1)))
> > > -	   || POINTER_TYPE_P (TREE_TYPE (gimple_assign_rhs1 (gs1)))))
> > > -    return false;
> > > -
> > >    tcode0 = gimple_assign_rhs_code (gs0);
> > >    tcode1 = gimple_assign_rhs_code (gs1);
> > >    if (TREE_CODE_CLASS (tcode0) == tcc_comparison
> > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > > index c8bee3b..db4d190 100644
> > > --- a/gcc/config/aarch64/aarch64.c
> > > +++ b/gcc/config/aarch64/aarch64.c
> > > @@ -12398,6 +12398,18 @@ aarch64_gen_ccmp_first (rtx *prep_seq, rtx *gen_seq,
> > >        icode = CODE_FOR_cmpdi;
> > >        break;
> > >
> > > +    case SFmode:
> > > +      cmp_mode = SFmode;
> > > +      cc_mode = aarch64_select_cc_mode ((rtx_code) code, op0, op1);
> > > +      icode = cc_mode == CCFPEmode ? CODE_FOR_fcmpesf : CODE_FOR_fcmpsf;
> > > +      break;
> > > +
> > > +    case DFmode:
> > > +      cmp_mode = DFmode;
> > > +      cc_mode = aarch64_select_cc_mode ((rtx_code) code, op0, op1);
> > > +      icode = cc_mode == CCFPEmode ? CODE_FOR_fcmpedf : CODE_FOR_fcmpdf;
> > > +      break;
> > > +
> > >      default:
> > >        end_sequence ();
> > >        return NULL_RTX;
> > > @@ -12461,6 +12473,18 @@ aarch64_gen_ccmp_next (rtx *prep_seq, rtx *gen_seq, rtx prev, int cmp_code,
> > >        icode = CODE_FOR_ccmpdi;
> > >        break;
> > >
> > > +    case SFmode:
> > > +      cmp_mode = SFmode;
> > > +      cc_mode = aarch64_select_cc_mode ((rtx_code) cmp_code, op0, op1);
> > > +      icode = cc_mode == CCFPEmode ? CODE_FOR_fccmpesf : CODE_FOR_fccmpsf;
> > > +      break;
> > > +
> > > +    case DFmode:
> > > +      cmp_mode = DFmode;
> > > +      cc_mode = aarch64_select_cc_mode ((rtx_code) cmp_code, op0, op1);
> > > +      icode = cc_mode == CCFPEmode ? CODE_FOR_fccmpedf : CODE_FOR_fccmpdf;
> > > +      break;
> > > +
> > >      default:
> > >        end_sequence ();
> > >        return NULL_RTX;
> > > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> > > index fab65c6..7d728b5 100644
> > > --- a/gcc/config/aarch64/aarch64.md
> > > +++ b/gcc/config/aarch64/aarch64.md
> > > @@ -279,6 +279,36 @@
> > >    [(set_attr "type" "alus_sreg,alus_imm,alus_imm")]
> > >  )
> > >
> > > +(define_insn "fccmp<mode>"
> > > +  [(set (match_operand:CCFP 1 "cc_register" "")
> > > +	(if_then_else:CCFP
> > > +	  (match_operator 4 "aarch64_comparison_operator"
> > > +	   [(match_operand 0 "cc_register" "")
> > > +	    (const_int 0)])
> > > +	  (compare:CCFP
> > > +	    (match_operand:GPF 2 "register_operand" "w")
> > > +	    (match_operand:GPF 3 "register_operand" "w"))
> > > +	  (match_operand 5 "immediate_operand")))]
> > > +  "TARGET_FLOAT"
> > > +  "fccmp\\t%<s>2, %<s>3, %k5, %m4"
> > > +  [(set_attr "type" "fcmp<s>")]
> > > +)
> > > +
> > > +(define_insn "fccmpe<mode>"
> > > +  [(set (match_operand:CCFPE 1 "cc_register" "")
> > > +	 (if_then_else:CCFPE
> > > +	  (match_operator 4 "aarch64_comparison_operator"
> > > +	   [(match_operand 0 "cc_register" "")
> > > +	  (const_int 0)])
> > > +	   (compare:CCFPE
> > > +	    (match_operand:GPF 2 "register_operand" "w")
> > > +	    (match_operand:GPF 3 "register_operand" "w"))
> > > +	  (match_operand 5 "immediate_operand")))]
> > > +  "TARGET_FLOAT"
> > > +  "fccmpe\\t%<s>2, %<s>3, %k5, %m4"
> > > +  [(set_attr "type" "fcmp<s>")]
> > > +)
> > > +
> > >  ;; Expansion of signed mod by a power of 2 using CSNEG.
> > >  ;; For x0 % n where n is a power of 2 produce:
> > >  ;; negs   x1, x0
> > > @@ -2794,7 +2824,7 @@
> > >    [(set_attr "type" "alus_sreg,alus_imm,alus_imm")]
> > >  )
> > >
> > > -(define_insn "*cmp<mode>"
> > > +(define_insn "fcmp<mode>"
> > >    [(set (reg:CCFP CC_REGNUM)
> > >          (compare:CCFP (match_operand:GPF 0 "register_operand" "w,w")
> > >  		      (match_operand:GPF 1 "aarch64_fp_compare_operand" "Y,w")))]
> > > @@ -2805,7 +2835,7 @@
> > >    [(set_attr "type" "fcmp<s>")]
> > >  )
> > >
> > > -(define_insn "*cmpe<mode>"
> > > +(define_insn "fcmpe<mode>"
> > >    [(set (reg:CCFPE CC_REGNUM)
> > >          (compare:CCFPE (match_operand:GPF 0 "register_operand" "w,w")
> > >  		       (match_operand:GPF 1 "aarch64_fp_compare_operand" "Y,w")))]
> > > diff --git a/gcc/testsuite/gcc.target/aarch64/ccmp_1.c b/gcc/testsuite/gcc.target/aarch64/ccmp_1.c
> > > new file mode 100644
> > > index 0000000..ef077e0
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/aarch64/ccmp_1.c
> > > @@ -0,0 +1,84 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-O2" } */
> > > +
> > > +int
> > > +f1 (int a)
> > > +{
> > > +  return a == 17 || a == 32;
> > > +}
> > > +
> > > +int
> > > +f2 (int a)
> > > +{
> > > +  return a == 33 || a == 18;
> > > +}
> > > +
> > > +int
> > > +f3 (int a, int b)
> > > +{
> > > +  return a == 19 && b == 34;
> > > +}
> > > +
> > > +int
> > > +f4 (int a, int b)
> > > +{
> > > +  return a == 35 && b == 20;
> > > +}
> > > +
> > > +int
> > > +f5 (int a)
> > > +{
> > > +  return a == 0 || a == 5;
> > > +}
> > > +
> > > +int
> > > +f6 (int a)
> > > +{
> > > +  return a == 6 || a == 0;
> > > +}
> > > +
> > > +int
> > > +f7 (int a, int b)
> > > +{
> > > +  return a == 0 && b == 7;
> > > +}
> > > +
> > > +int
> > > +f8 (int a, int b)
> > > +{
> > > +  return a == 9 && b == 0;
> > > +}
> > > +
> > > +int
> > > +f9 (float a, float b)
> > > +{
> > > +  return a < 0.0f && a > b;
> > > +}
> > > +
> > > +int
> > > +f10 (float a, float b)
> > > +{
> > > +  return a == b || b == 0.0f;
> > > +}
> > > +
> > > +int
> > > +f11 (double a, int b)
> > > +{
> > > +  return a < 0.0f && b == 30;
> > > +}
> > > +
> > > +int
> > > +f12 (double a, int b)
> > > +{
> > > +  return b == 31 || a == 0.0f;
> > > +}
> > > +
> > > +int
> > > +f13 (int a, int b)
> > > +{
> > > +  a += b;
> > > +  return a == 3 || a == 0;
> > > +}
> > > +
> > > +/* { dg-final { scan-assembler "fccmp\t" } } */
> > > +/* { dg-final { scan-assembler "fccmpe\t" } } */
> > > --
> > > 1.9.1
> >