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.tsa...@vrull.eu> wrote: > > On Mon, Jul 3, 2023 at 12:12 PM Robin Dapp <rdapp....@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 > >
