[PATCH] D159533: [DAG] getNode() - fold (zext (trunc x)) -> x iff the upper bits are known zero - add SRL support

[Jeffrey Byrnes via Phabricator via llvm-commits]

jrbyrnes added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:10791
+                                       Depth + 1, StartingIndex)
+               : calculateByteProvider(Op.getOperand(0),
+                                       Index - (BytesProvided - ByteShift),
----------------
jrbyrnes wrote:
> This seems to be broken if Index = 1; BytesProvided; = 2 and ByteShift = 3 -- we will attempt to get the 2nd byte of a 16 bit operand which is out of range (only 0,1 are valid bytes).
> 
> Perhaps it was meant to be: `Index + (BytesProvided - ByteShift)` ? But this still doesnt account for the case where `ByteShift > n * BytesProvided`. 
> 
> 
> How do you feel about
> 
> 
> ```
>   case ISD::FSHR: {
>     auto ShiftOp = dyn_cast<ConstantSDNode>(Op->getOperand(2));
>     if (!ShiftOp || Op.getValueType().isVector())
>       return std::nullopt;
> 
>     uint64_t BitsProvided = Op.getValueSizeInBits();
>     if (BitsProvided % 8 != 0)
>       return std::nullopt;
> 
>     uint64_t BitShift = ShiftOp->getAPIntValue().urem(BitsProvided);
>     if (BitShift % 8)
>       return std::nullopt;
> 
>     uint64_t ConcatSizeInBytes = BitsProvided / 4;
>     uint64_t ByteShift = BitShift / 8;
> 
>     auto NewIndex = (Index + ByteShift) % ConcatSizeInBytes;
>     auto BytesProvided = BitsProvided / 8;
>     auto NextOp = Op.getOperand(NewIndex >= BytesProvided ? 0 : 1);
>     NewIndex %= BytesProvided;
>     return calculateSrcByte(NextOp, StartingIndex, NewIndex);
>   }
> ```
Oops -- should have been

`return calculateByteProvider(NextOp, NewIndex, Depth + 1, StartingIndex`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159533