Re: [PATCH v3 2/4] ifcvt: Allow more operations in multiple set if conversion

[Manolis Tsamis <manolis.tsamis@vrull.eu>]

On Thu, Oct 19, 2023 at 10:46 PM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Manolis Tsamis <manolis.tsamis@vrull.eu> writes:
> > Currently the operations allowed for if conversion of a basic block with
> > multiple sets are few, namely REG, SUBREG and CONST_INT (as controlled by
> > bb_ok_for_noce_convert_multiple_sets).
> >
> > This commit allows more operations (arithmetic, compare, etc) to participate
> > in if conversion. The target's profitability hook and ifcvt's costing is
> > expected to reject sequences that are unprofitable.
> >
> > This is especially useful for targets which provide a rich selection of
> > conditional instructions (like aarch64 which has cinc, csneg, csinv, ccmp, ...)
> > which are currently not used in basic blocks with more than a single set.
> >
> > gcc/ChangeLog:
> >
> >       * ifcvt.cc (try_emit_cmove_seq): Modify comments.
> >       (noce_convert_multiple_sets_1): Modify comments.
> >       (bb_ok_for_noce_convert_multiple_sets): Allow more operations.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       * gcc.target/aarch64/ifcvt_multiple_sets_arithm.c: New test.
> >
> > Signed-off-by: Manolis Tsamis <manolis.tsamis@vrull.eu>
> > ---
> >
> > Changes in v3:
> >         - Add SCALAR_INT_MODE_P check in bb_ok_for_noce_convert_multiple_sets.
> >         - Allow rewiring of multiple regs.
> >         - Refactor code with noce_multiple_sets_info.
> >         - Remove old code for subregs.
> >
> >  gcc/ifcvt.cc                                  | 63 ++++++++++-----
> >  .../aarch64/ifcvt_multiple_sets_arithm.c      | 79 +++++++++++++++++++
> >  2 files changed, 123 insertions(+), 19 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.target/aarch64/ifcvt_multiple_sets_arithm.c
> >
> > diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc
> > index 3273aeca125..efe8ab1577a 100644
> > --- a/gcc/ifcvt.cc
> > +++ b/gcc/ifcvt.cc
> > @@ -3215,13 +3215,13 @@ try_emit_cmove_seq (struct noce_if_info *if_info, rtx temp,
> >  /* We have something like:
> >
> >       if (x > y)
> > -       { i = a; j = b; k = c; }
> > +       { i = EXPR_A; j = EXPR_B; k = EXPR_C; }
> >
> >     Make it:
> >
> > -     tmp_i = (x > y) ? a : i;
> > -     tmp_j = (x > y) ? b : j;
> > -     tmp_k = (x > y) ? c : k;
> > +     tmp_i = (x > y) ? EXPR_A : i;
> > +     tmp_j = (x > y) ? EXPR_B : j;
> > +     tmp_k = (x > y) ? EXPR_C : k;
> >       i = tmp_i;
> >       j = tmp_j;
> >       k = tmp_k;
> > @@ -3637,11 +3637,10 @@ noce_convert_multiple_sets_1 (struct noce_if_info *if_info,
> >
> >
> >
> > -/* Return true iff basic block TEST_BB is comprised of only
> > -   (SET (REG) (REG)) insns suitable for conversion to a series
> > -   of conditional moves.  Also check that we have more than one set
> > -   (other routines can handle a single set better than we would), and
> > -   fewer than PARAM_MAX_RTL_IF_CONVERSION_INSNS sets.  While going
> > +/* Return true iff basic block TEST_BB is suitable for conversion to a
> > +   series of conditional moves.  Also check that we have more than one
> > +   set (other routines can handle a single set better than we would),
> > +   and fewer than PARAM_MAX_RTL_IF_CONVERSION_INSNS sets.  While going
> >     through the insns store the sum of their potential costs in COST.  */
> >
> >  static bool
> > @@ -3667,20 +3666,46 @@ bb_ok_for_noce_convert_multiple_sets (basic_block test_bb, unsigned *cost)
> >        rtx dest = SET_DEST (set);
> >        rtx src = SET_SRC (set);
> >
> > -      /* We can possibly relax this, but for now only handle REG to REG
> > -      (including subreg) moves.  This avoids any issues that might come
> > -      from introducing loads/stores that might violate data-race-freedom
> > -      guarantees.  */
> > -      if (!REG_P (dest))
> > +      /* Do not handle anything involving memory loads/stores since it might
> > +      violate data-race-freedom guarantees.  */
> > +      if (!REG_P (dest) || contains_mem_rtx_p (src))
> > +     return false;
> > +
> > +      if (!SCALAR_INT_MODE_P (GET_MODE (src)))
> >       return false;
> >
> > -      if (!((REG_P (src) || CONSTANT_P (src))
> > -         || (GET_CODE (src) == SUBREG && REG_P (SUBREG_REG (src))
> > -           && subreg_lowpart_p (src))))
> > +      /* Allow a wide range of operations and let the costing function decide
> > +      if the conversion is worth it later.  */
> > +      enum rtx_code code = GET_CODE (src);
> > +      if (!(CONSTANT_P (src)
> > +         || code == REG
> > +         || code == SUBREG
> > +         || code == ZERO_EXTEND
> > +         || code == SIGN_EXTEND
> > +         || code == NOT
> > +         || code == NEG
> > +         || code == PLUS
> > +         || code == MINUS
> > +         || code == AND
> > +         || code == IOR
> > +         || code == MULT
> > +         || code == ASHIFT
> > +         || code == ASHIFTRT
> > +         || code == NE
> > +         || code == EQ
> > +         || code == GE
> > +         || code == GT
> > +         || code == LE
> > +         || code == LT
> > +         || code == GEU
> > +         || code == GTU
> > +         || code == LEU
> > +         || code == LTU
> > +         || code == COMPARE))
> >       return false;
>
> I'm nervous about lists of operations like these, for two reasons:
>
> (1) It isn't obvious what criteria are used to select the codes.
>
> (2) It requires the top-level code to belong to a given set, but it
>     allows subrtxes of src to be arbitrarily complex.  E.g. (to pick
>     a silly example) a toplevel (popcount ...) would be rejected, but
>     (plus (popcount ...) (const_int 1)) would be OK.
>
> Could we just remove this condition instead?
>
True and done: https://gcc.gnu.org/pipermail/gcc-patches/2024-April/649899.html

I also removed the if (!SCALAR_INT_MODE_P (GET_MODE (src))) because it
was rejecting (const_int) apparently. The mode is checked in the rest
of the code so it wasn't needed in the first place.

Manolis

> Thanks,
> Richard
>
> >
> > -      /* Destination must be appropriate for a conditional write.  */
> > -      if (!noce_operand_ok (dest))
> > +      /* Destination and source must be appropriate.  */
> > +      if (!noce_operand_ok (dest) || !noce_operand_ok (src))
> >       return false;
> >
> >        /* We must be able to conditionally move in this mode.  */
> > diff --git a/gcc/testsuite/gcc.target/aarch64/ifcvt_multiple_sets_arithm.c b/gcc/testsuite/gcc.target/aarch64/ifcvt_multiple_sets_arithm.c
> > new file mode 100644
> > index 00000000000..d977f4d62ec
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/ifcvt_multiple_sets_arithm.c
> > @@ -0,0 +1,79 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-rtl-ce1" } */
> > +
> > +void sink2(int, int);
> > +void sink3(int, int, int);
> > +
> > +void cond1(int cond, int x, int y)
> > +{
> > +  if (cond)
> > +    {
> > +      x = x << 4;
> > +      y = 1;
> > +    }
> > +
> > +  sink2(x, y);
> > +}
> > +
> > +void cond2(int cond, int x, int y)
> > +{
> > +  if (cond)
> > +    {
> > +      x++;
> > +      y++;
> > +    }
> > +
> > +  sink2(x, y);
> > +}
> > +
> > +void cond3(int cond, int x1, int x2, int x3)
> > +{
> > +  if (cond)
> > +    {
> > +      x1++;
> > +      x2++;
> > +      x3++;
> > +    }
> > +
> > +  sink3(x1, x2, x3);
> > +}
> > +
> > +void cond4(int cond, int x, int y)
> > +{
> > +  if (cond)
> > +    {
> > +      x += 2;
> > +      y += 3;
> > +    }
> > +
> > +  sink2(x, y);
> > +}
> > +
> > +void cond5(int cond, int x, int y, int r1, int r2)
> > +{
> > +  if (cond)
> > +    {
> > +      x = r1 + 2;
> > +      y = r2 - 34;
> > +    }
> > +
> > +  sink2(x, y);
> > +}
> > +
> > +void cond6(int cond, int x, int y)
> > +{
> > +  if (cond)
> > +    {
> > +      x = -x;
> > +      y = ~y;
> > +    }
> > +
> > +  sink2(x, y);
> > +}
> > +
> > +/* { dg-final { scan-assembler-times "cinc\t" 5 } } */
> > +/* { dg-final { scan-assembler-times "csneg\t" 1 } } */
> > +/* { dg-final { scan-assembler-times "csinv\t" 1 } } */
> > +/* { dg-final { scan-assembler "csel\t" } } */
> > +
> > +/* { dg-final { scan-rtl-dump-times "if-conversion succeeded through noce_convert_multiple_sets" 6 "ce1" } } */
> > \ No newline at end of file