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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4018
+
+/// Form a G_UBFX from "(a srl b) & mask", where b and mask are constants.
 bool CombinerHelper::matchBitfieldExtractFromAnd(
----------------
You've now got different doxygen comments on the declaration and definition of this function.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:2045
+
+    if (TypeIdx == 0) {
+      widenScalarSrc(MI, WideTy, 1, TargetOpcode::G_ANYEXT);
----------------
Should this be `TypeIdx == 0 || TypeIdx == 1`?


================
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
----------------
I think this needs to be `<= 32`. At least, the `else` case won't correctly handle a signed extract with width == 32.


================
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);
----------------
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.


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

https://reviews.llvm.org/D100149