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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope 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");
+
----------------
foad wrote:
> 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!
No worries. 
Patch seem good in general. I saw some lowering improvements for important cases (and only some minor regression, at least in instruction count, for weird types like i37 but that isn't really important for my backend).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77152