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

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

I resent this with just the change in the comment.
OK to merge?

Manolis

On Tue, Jul 4, 2023 at 5:32 PM Manolis Tsamis <manolis.tsamis@vrull.eu> wrote:
>
> 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
> >