[PATCH] D87847: [AMDGPU] global-isel support for RT

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:3059-3075
+  if (IsA16) {
+    MIB.addReg(RayDir, 0, AMDGPU::sub0);
+    Register R1 = MRI->createVirtualRegister(&AMDGPU::VGPR_32RegClass);
+    Register R2 = MRI->createVirtualRegister(&AMDGPU::VGPR_32RegClass);
+    BuildMI(*MBB, &*MIB, DL, TII.get(AMDGPU::V_PACK_B32_F16), R1)
+      .addImm(0)
+      .addReg(RayDir, 0, AMDGPU::sub1)
----------------
rampitec wrote:
> arsenm wrote:
> > Can you do this during custom lowering rather than adding bit operations here late? I'm also surprised a V_PACK_B32_F16 is involved here
> What kind of operations you'd like to see in the custom lowering? v_pack_b32_f16 should be fine, this is packed half type in this case.
But v_pack_b32_f16 isn't semantically the same as the bit packing, so I would be surprised to insert this for the argument handling.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:4389
+      OpdsMapping[6] = AMDGPU::getValueMapping(AMDGPU::VGPRRegBankID, DirSize);
+      OpdsMapping[7] = AMDGPU::getValueMapping(AMDGPU::SGPRRegBankID, 128);
+      break;
----------------
rampitec wrote:
> arsenm wrote:
> > Missing waterfall loop for SGPR operand
> That's a descriptor, I'd rather refuse to select.
You can never guarantee the input is uniform or in a VGPR. We can do the right thing now easily (and every other intrinsic with a descriptor does it)


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

https://reviews.llvm.org/D87847