[PATCH] D100149: [AMDGPU][GlobalISel] Legalize and select G_SBFX and G_UBFX

[Brendon Cahoon via Phabricator via llvm-commits]

bcahoon marked 4 inline comments as done.
bcahoon added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1613
+    auto AndHi = B.buildAnd(S32, UnmergeSOffset.getReg(1), SubHi);
+    B.buildMerge(DstReg, {AndLo, AndHi});
+    MI.eraseFromParent();
----------------
foad wrote:
> arsenm wrote:
> > foad wrote:
> > > Looks like you're missing the sign extension here for the sbfe/sbfx case?
> > Yes, this looks suspicious
> I would suggest that if width is not a constant, you instead expand to `Src >> Offset << (64 - Width) >> (64 - Width)` where the last shift is signed for G_SBFX, and the first shift might disappear if Offset happens to be 0. I'm assuming the result is undefined if Width is not in the range 1..BitWidth inclusive but the definition of these opcodes does not make that clear.
Thanks for pointing this out and for the suggestion. I've implemented your suggestion.


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

https://reviews.llvm.org/D100149