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

[Brendon Cahoon via Phabricator via llvm-commits]

bcahoon marked an inline comment as done.
bcahoon added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:2045
+
+    if (TypeIdx == 0) {
+      widenScalarSrc(MI, WideTy, 1, TargetOpcode::G_ANYEXT);
----------------
foad wrote:
> Should this be `TypeIdx == 0 || TypeIdx == 1`?
I believe that TypeIdx 0 is for the destination and source operands.  And, TypeIdx 1 is for the offset and width operands, which should be zero extended.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1576
+      auto WidthImm = ConstWidth->Value.getZExtValue();
+      if (WidthImm < 32) {
+        // Use bitfield extract on the lower 32-bit source, and then sign-extend
----------------
foad wrote:
> I think this needs to be `<= 32`. At least, the `else` case won't correctly handle a signed extract with width == 32.
Thanks for point this out!


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1601
+    // operations. This assumes the operation has undefined behavior if Width
+    // is greater than 64 bits.
+    auto ExtShift = B.buildSub(S32, B.buildConstant(S32, 64), WidthReg);
----------------
foad wrote:
> The fact that it's undefined if Width>64 is not really surprising.
> 
> The things that might be surprising are (a) it's defined if Width==64 and (b) it's undefined if Width==0. Especially the latter, since some people might expect to be able to do an unsigned zero-width extract and rely on the result being zero.
> 
> TODO: make these rules clear in the definition of the G_*BFX opcodes.
Ah right, good point. I've updated the comment and added the TODO. Let me know if you'd prefer more details in the comment. I can follow up with another PR to update the G_SBFX/G_UBFX opcodes.


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

https://reviews.llvm.org/D100149