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

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

On Mon, Jul 3, 2023 at 12:12 PM Robin Dapp <rdapp.gcc@gmail.com> wrote:
>
> Hi Manolis,
>
> that looks like a nice enhancement of what's already possible.  The concern
> I had some years back already was that this function would eventually
> grow and cannibalize on some of what the other functions in ifcvt already
> do :)  At some point we really should unify but that's not within the
> scope of this patch.
>

Hi Robin,

Indeed and it would be nice to extend the multi statement
implementation to the point that the others are not needed :)
I have some future plans to analyze cases where the multi-statement
performs worse and improve on that.

> IMHO we're already pretty far towards general "conditional execution"
> with conditional increments, selects and so on (and the function is still
> called "_noce") and historically the cond_exec functions would have
> taken care of that.  To my knowledge though, none of the major backends
> implements anything like (cond_exec ...) anymore and relies on bit-twiddling
> tricks to generate the conditional instructions.
>
> Have you checked whether cond_exec and others could be adjusted to
> handle the conditional instructions you want to see?  They don't perform
> full cost comparison though but just count.
>

Thanks for mentioning that, I was not really aware of cond_exec usage.
As you say, it looks like cond_exec isn't used very much on major backends.

Since noce_convert_multiple_sets_1 is just using the existing ifcvt
machinery (specifically noce_emit_cmove / try_emit_cmove_seq), is this
a question of whether we want to replace (if_then_else ...) with
(cond_exec ...) in general?
If that is beneficial then I could try to implement a change like
this, but that should probably be a separate effort from this
implementation.

> I would expect a bit of discussion around that but from a first look
> I don't have major concerns.
>
> > -/* 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
>
> Might want to change the "conditional moves" while you're at it.
>

Thanks for pointing out this comment, I missed it. I will rewrite the
relevant parts.

> >
> > -      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))
>
> We're potentially checking many more patterns than before.  Maybe it
> would make sense to ask the backend whether it has a pattern for
> the respective code?
>

Is it an issue if the backend doesn't have a pattern for a respective code?

My goal here is to not limit if conversion for sequences based on the
code but rather let ifcvt / the backedn decide based on costing.
That's because from what I've seen, conditional set instructions can
be beneficial even when the backend doesn't have a specific
instruction for that code.

Best,
Manolis

> Regards
>  Robin
>