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 >
