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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

Should have some end to end IR tests for all of these. Also would like to see if the constant cases get appropriately constant folded without special casing it. Another case to look out for is when the offset is known if we can reduce the 64-bit shift to 32-bits (it won't happen now, but theoretically it should trigger in the post-regbank combiner)



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:1637
+      .clampScalar(0, S32, S64)
+      .widenScalarToNextPow2(0);
+
----------------
Also needs to scalarize vectors


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1532
+    ApplyRegBankMapping ApplyBank(*this, MRI, &AMDGPU::VGPRRegBank);
+    MachineIRBuilder B(MI, ApplyBank);
 
----------------
I don't like creating single use MachineIRBuilders, but you have to do this right now


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1555
+    // Produce the extracted bitfield.
+    B.buildAnd(S64, ShiftOffset, Merge);
+    MI.eraseFromParent();
----------------
This dropped the result register


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/inst-select-sbfx.mir:65
+    S_ENDPGM 0, implicit %3
+...
----------------
Doesn't cover the scalar cases


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/inst-select-ubfx.mir:3
+# RUN: llc -march=amdgcn -mcpu=fiji -run-pass=instruction-select -verify-machineinstrs -o - %s | FileCheck -check-prefix=WAVE64 %s
+# RUN: llc -march=amdgcn -mcpu=gfx1010 -run-pass=instruction-select -verify-machineinstrs -o - %s | FileCheck -check-prefix=WAVE32 %s
+
----------------
There's no real reason to check both wave sizes here


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/inst-select-ubfx.mir:65
+    S_ENDPGM 0, implicit %3
+...
----------------
Doesn't cover the scalar cases


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-ubfx.mir:69
+---
+name:            test_ubfx_s8
+body: |
----------------
Should also test s16 which is more interesting. Also some vectors, in particular <2 x s32>, <2 x s16>, <3 x s16> and <4 x s16>


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/regbankselect-ubfx.mir:26
+    %3:_(s32) = G_UBFX %0, %1(s32), %2
+...
+
----------------
You can technically get away with this now, but I would prefer to have a use for all of the results. The other passes all tend to DCE instructions as they go, and someday regbankselect may start doing this


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

https://reviews.llvm.org/D100149