[PATCH] D109301: [AMDGPU] Enable copy between VGPR and AGPR classes during regalloc

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1054
+    // needs a copy in such cases.
+    auto CopyMIB = BuildMI(MBB, MI, DL, TII->get(AMDGPU::COPY), Dst)
+                       .addReg(Src, getKillRegState(IsKill));
----------------
cdevadas wrote:
> rampitec wrote:
> > We are already spilling. I.e. we ran out of registers of ValueReg's class and we know it. Why cannot we just copy into a different class instead of introducing an ambiguous AV class?
> I didn't follow the question. Are you, in the first place, doubting the need for AV class as a superclass?
No, I just mean it is not needed right here. In particular that is the reason for regression in the spill-to-agpr-partial.mir.


================
Comment at: llvm/test/CodeGen/AMDGPU/spill-to-agpr-partial.mir:234
+    ; GCN: {{  $}}
+    ; GCN: $vgpr55 = V_ACCVGPR_READ_B32_e64 killed $agpr3, implicit $exec, implicit-def $agpr0_agpr1_agpr2_agpr3, implicit $agpr0_agpr1_agpr2_agpr3
+    ; GCN: SCRATCH_STORE_DWORDX3_SADDR killed $agpr0_agpr1_agpr2, $sgpr32, 0, 0, implicit $exec, implicit $flat_scr, implicit killed $agpr0_agpr1_agpr2_agpr3 :: (store (s96) into %stack.0, align 4, addrspace 5)
----------------
cdevadas wrote:
> rampitec wrote:
> > This should not happen. This is gfx90a and agpr has to be stored directly. This should happen on gfx908 though.
> You are right. The original test was for gfx90a. 
> The new patterns should be in a separate test for gfx908. I will create one.
But this copy still should not happen on gfx90a as it does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109301