[PATCH] D25428: AMDGPU add support for spilling to a user data SREG address.

[Matt Arsenault via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIFrameLowering.cpp:245-270
+    if (ST.hasSpillUserPtr()) {
+      unsigned Rsrc01 = TRI->getSubReg(ScratchRsrcReg, AMDGPU::sub0_sub1);
+      const MCInstrDesc &MoveDwordX2 = TII->get(AMDGPU::S_LOAD_DWORDX2_IMM);
+      unsigned sgpr01 = TRI->getMatchingSuperReg(AMDGPU::SGPR0, AMDGPU::sub0, &AMDGPU::SReg_64RegClass);
+      MRI.addLiveIn(AMDGPU::SGPR0);
+      MBB.addLiveIn(AMDGPU::SGPR0);
+
----------------
Formatting for this all looks wrong


================
Comment at: lib/Target/AMDGPU/SIFrameLowering.cpp:247
+      unsigned Rsrc01 = TRI->getSubReg(ScratchRsrcReg, AMDGPU::sub0_sub1);
+      const MCInstrDesc &MoveDwordX2 = TII->get(AMDGPU::S_LOAD_DWORDX2_IMM);
+      unsigned sgpr01 = TRI->getMatchingSuperReg(AMDGPU::SGPR0, AMDGPU::sub0, &AMDGPU::SReg_64RegClass);
----------------
This is confusingly named because it's not S_MOV_B64


================
Comment at: lib/Target/AMDGPU/SIFrameLowering.cpp:248
+      const MCInstrDesc &MoveDwordX2 = TII->get(AMDGPU::S_LOAD_DWORDX2_IMM);
+      unsigned sgpr01 = TRI->getMatchingSuperReg(AMDGPU::SGPR0, AMDGPU::sub0, &AMDGPU::SReg_64RegClass);
+      MRI.addLiveIn(AMDGPU::SGPR0);
----------------
You can just use the literal AMDGPU::SGPR0_SGPR1 if you're just using a known constant register. However, I would expect this to be getting the register from the MachineFunctionInfo rather than hardcoding it again inside FrameLowering


================
Comment at: lib/Target/AMDGPU/SIFrameLowering.cpp:253
+      BuildMI(MBB, I, DL, MoveDwordX2, Rsrc01)
+	.addReg(sgpr01, RegState::Undef)
+	.addImm(0);
----------------
This shouldn't be undef, the read value does matter


================
Comment at: lib/Target/AMDGPU/SIFrameLowering.cpp:256
+      BuildMI(MBB, I, DL, MoveDwordX2, Rsrc01)
+	.addReg(Rsrc01, RegState::Undef, AMDGPU::sub0_sub1)
+	.addImm(0);
----------------
Because this is using physical registers here, the sub register index isn't used (also sub0_sub1 of s[0:1]  doesn't make sense)


================
Comment at: lib/Target/AMDGPU/SIFrameLowering.cpp:256
+      BuildMI(MBB, I, DL, MoveDwordX2, Rsrc01)
+	.addReg(Rsrc01, RegState::Undef, AMDGPU::sub0_sub1)
+	.addImm(0);
----------------
arsenm wrote:
> Because this is using physical registers here, the sub register index isn't used (also sub0_sub1 of s[0:1]  doesn't make sense)
Because you are creating a load you should add a MachineMemOperand or else it will unduly constrain the post-RA scheduler. There are a few other dummy places that create read-only memory operands for similar purposes.

Also because it's a load I would hope we could do this much earlier than frame lowering. I'm thinking about how to properly initialize m0 in the HSA ABI which also requires a load so I will probably end up taking care of that eventually 


Repository:
  rL LLVM

https://reviews.llvm.org/D25428