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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:2039
+
+    if (TypeIdx == 0 ) {
+      widenScalarSrc(MI, WideTy, 1, TargetOpcode::G_ANYEXT);
----------------
Extra space before )


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:770
+  const RegisterBank *DstRB = RBI.getRegBank(DstReg, *MRI, TRI);
+  // The scslar bitfield extract instructions pack the offset and width into a
+  // 32-bit value.
----------------
Typo scslar


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:780-800
+    // Zero the high bits of the register with the offset so that it can be
+    // combined with the width.
+    auto MIB = BuildMI(*MBB, &MI, DL, TII.get(AMDGPU::S_AND_B32), TmpReg0)
+                   .addReg(OffsetReg)
+                   .addImm(maskTrailingOnes<unsigned>(6));
+    constrainSelectedInstRegOperands(*MIB, TII, TRI, RBI);
+    // Shift the width to the high 16-bits.
----------------
It would be better to handle this operand packing in RegBankSelect applyMappingImpl. We can get simplifications after the fact if you emit the generic bit operations. If the offset/width are really constants, this won't end up constant folding them (actually today SIFoldOperands should constant fold them, but the only reason it tries to do this is a hack for SelectionDAG)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100149