[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:
> rampitec wrote:
> > arsenm wrote:
> > > rampitec wrote:
> > > > arsenm wrote:
> > > > > 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.
> > > > That's the best instruction for the job IMO. What we are doing is repacking vector of halfs.
> > > But it does change the input values. I believe this is a canonicalizing operation, so may flush denorms and quiet snans
> > It should behave the same as bhv itself, the mode is common right? So if flushing on the value will be flushed anyway. Everything else results in a longer code. It can use v_lshl_or_b32, but it will also need an extra v_and_b32 to clear high half.
> Doing a custom lowering would need 4 different custom nodes and then selection. It will be much more overhead.
You could use one wrapper instruction like the image intrinsics. We should expose the bit packing to the post-legalize combiner


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

https://reviews.llvm.org/D87847