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

[Jay Foad via Phabricator via llvm-commits]

foad added reviewers: aemerson, paquette.
foad added a comment.

LGTM for AMDGPU, and I guess you haven't changed the behaviour for AArch64. What about other GlobalISel targets like MIPS and X86? Will they suddenly start seeing G_SBFX/G_UBSX where they weren't expecting them? Anyway I've added a couple more globalisel reviewers.



================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:2045
+
+    if (TypeIdx == 0) {
+      widenScalarSrc(MI, WideTy, 1, TargetOpcode::G_ANYEXT);
----------------
bcahoon wrote:
> 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.
Oh yes, you're right. I had not realized that these type indices refer back to "type0" and "type1" in the definition of G_SBFX/G_UBFX.


================
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);
----------------
bcahoon wrote:
> 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.
> I can follow up with another PR to update the G_SBFX/G_UBFX opcodes.

Yes please!


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

https://reviews.llvm.org/D100149