[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:6163
+  unsigned RevOpcode = IsFSHL ? ISD::FSHR : ISD::FSHL;
+  if (!isOperationLegalOrCustom(Node->getOpcode(), VT) &&
+      isOperationLegalOrCustom(RevOpcode, VT)) {
----------------
bjope wrote:
> bjope wrote:
> > Is this transform even correct?
> > 
> > Assume we got `fshl 3, 2, 0`, then the result would be 3. But this code could turn it into `fshr 3, 2, (0-0)`, then the result would become 2.
> Maybe one can limit the condition to
> 
> ```
> if (!isOperationLegalOrCustom(Node->getOpcode(), VT) &&
>       isOperationLegalOrCustom(RevOpcode, VT) &&
>       isPowerOf2_32(BW) && isNonZeroModBitWidth(Z, BW)) {
> ```
> 
> Although, isNonZeroModBitWidth currently allow undef values. And I wonder if it is allowed to turn `fshl X, Y, undef` into `fshr X, Y, (0-UNDEF)` , considering that we for example know that `fshl -1, 0, undef` can't be zero, but `fshr -1, 0, undef` can be zero. So one probably needs to restrict the condition further by adding a isNonZeroNonUndefModBitWidth helper and use that here.
You're right. I'll take a proper look on Monday. Of course this is just an optimization so it's safe to disable it completely if it's causing problems. I think I added it to fix some code quality regressions in the AMDGPU tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77152