[PATCH] D78829: [AMDGPU] Make SREG_LO16 legal

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec marked 3 inline comments as done.
rampitec added a comment.

In D78829#2002912 <https://reviews.llvm.org/D78829#2002912>, @arsenm wrote:

> Could we leave all instructions as 32-bit defs, but then have a 16-bit subreg copy as the only use?
>
> %0:vgpr_32 = V_FOO_U16
>  %1:vgpr_16lo = COPY %0.sub16_lo
>  ....
>
> The register allocator would understand this and fold out the copy


So you want to keep SReg_32 as an RC for i16 and f16? And add a copy to each instruction producing a 16 bit value?

I am afraid it will not work. Instruction needs to produce i16 for which legal class is SReg_32, we will return SReg_lo16. Selector will either complain, not match or emit yet another copy. The problem is a 16 bit value needs to leave either in a 32 bit or a 16 bit RC, but not both.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp:315
 
+    if (MI->getOpcode() == AMDGPU::DELETED) {
+      if (isVerbose())
----------------
arsenm wrote:
> This is a red flag, this should never happen
This is in place of redundant mov v0, v0 when one side is 16 bit and another 32. When all is done it should go away.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:544-547
+    }
+
+    RC = RI.getPhysRegClass(DestReg);
+  }
----------------
arsenm wrote:
> We should make it so that MachineCopyPropagation can deal with this case
Hm... So you want it always? I thought this is a red flag.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:545
+    }
+
+    RC = RI.getPhysRegClass(DestReg);
----------------
arsenm wrote:
> Why can't you just erase the instruction?
Because caller decrements the iterator expecting new instruction to be there. It then transfers implicit operands there. This is also a hack and frankly breaks if we emit more than one instruction. Maybe we need to emit a bundle instead of a sequence.

This is also why DELETED is created.


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

https://reviews.llvm.org/D78829