[PATCH] D86449: [SelectionDAG] Handle non-power-of-2 bitwidths in expandROT

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

In D86449#2237244 <https://reviews.llvm.org/D86449#2237244>, @nikic wrote:

> In D86449#2236491 <https://reviews.llvm.org/D86449#2236491>, @spatel wrote:
>
>> Do we guarantee that the rotate amount is not undef? (Can we assert that?)
>> The expansion may fail if the rotate amount is 'undef':
>> https://alive2.llvm.org/ce/z/tqjuC7
>
> Undef is a legal rotate amount (because we spec rotate to be modulo the bitwidth, unlike normal shifts, where it is poison). However, I don't think it makes sense to consider undef-related issues at the SDAG layer at this point in time. They are much less likely to cause practical miscompiles here, and it's much harder to be undef-correct in SDAG (because expansion and legalization will often introduce multiple uses of the values, with the issues that may entail, as in this case). Being pedantic about this would need a lot of FREEZE.

Yes, I agree that we don't want to take undef in codegen too seriously. We may want to add that fold for safety though because it seems to survive on most targets here.

And I see now that we are currently crashing on this example (on all targets?), so we need to get this fixed ASAP.
LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86449/new/

https://reviews.llvm.org/D86449