[PATCH] D78772: [AMDGPU] Adapt GCNRegBankReassign for 16 bit subregs

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp:456-457
+  //       parent VGPR_32 and potentially a sibling 16 bit sub-register.
+  if (Size < 32)
+    return false;
+
----------------
arsenm wrote:
> This kills the code below
Yes, and it has TODO above it. These registers are still reassignable, it just needs more code.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1789-1790
+  const TargetRegisterClass *RC = getPhysRegClass(Reg);
+  if (getRegSizeInBits(*RC) != 16)
+    return Reg;
+
----------------
arsenm wrote:
> Seems like this should just be an assert
I am going to use it without checking is a register actually 16 or 32 bit, just to get a 32 bit operand from whatever input. But it is reasonable to add assert for "Size <= 32".


================
Comment at: llvm/test/CodeGen/AMDGPU/regbank-reassign.mir:374-378
+registers:
+  - { id: 0, class: vgpr_32, preferred-register: '$vgpr1' }
+  - { id: 1, class: vgpr_32, preferred-register: '$vgpr5' }
+  - { id: 2, class: vgpr_32 }
+  - { id: 3, class: vgpr_lo16 }
----------------
arsenm wrote:
> If you don't need the preferred-register hints you can eliminate the registers section
I intend to reuse these tests when registers become reassignable. I also want to trigger code path in the pass which will actually go through the bank search logic. If RA will just assign v0 and v1 not all code would trigger.


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

https://reviews.llvm.org/D78772