[PATCH] D77152: [SelectionDAG] Better legalization for FSHL and FSHR

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6159
 
+  assert(isPowerOf2_32(BW) && "Expecting the type bitwidth to be a power of 2");
+
----------------
bjope wrote:
> foad wrote:
> > bjope wrote:
> > > Our OOT target got legal types that aren't pow-of-2. So this kind of broke some funnel-shift tests that happened to work earlier.
> > > 
> > > I realize that it might be hard to verify the behavior for non-pow-of-2 types upstream, so not sure if I'm welcome to do patches upstream that fixes the problem (although there might be other OOT targets in the same situation).
> > > 
> > > BTW: I haven't figured out exactly what part of the code below that only works for pow-of-2 types yet. I see some checks for `isPowerOf2_32(BW)` further down in the code, so maybe the assert isn't needed at all?
> > > 
> > I believe that only the conversion fshl(X, Y, Z) <--> fshr(X, Y, -Z) depends on bitwidth being a power of two. So pushing the assertion inside the "if" might be a quick fix, assuming your target has equally good support for shifting in both directions?
> > 
> > The correct conversion for non-power-of-two would be something like: fshl(X, Y, Z) -> fshr(X, Y, BW - (Z % BW)) and vice versa. Personally I'd be happy to have this code upstream, though I don't know if there would be any way of testing it.
> Yes, I realize this is related to the new code below.
> So we could just add it as a condition in the if-statement related to swapping between FSHL<->FSHR.
> 
> Now it is a bit weird to have the assert here, guarding all the code below, when the general case below handle non-pow-of-2 BW explicitly.
> So we could just add it as a condition in the if-statement related to swapping between FSHL<->FSHR.

Good idea, maybe with a comment explaining how to implement properly.

> Now it is a bit weird to have the assert here, guarding all the code below, when the general case below handle non-pow-of-2 BW explicitly.

Agreed. Sorry about that!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77152