[PATCH] D25428: AMDGPU add support for spilling to a user data SREG address.
Matt Arsenault via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 13 06:17:43 PDT 2016
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
More information about the llvm-commits
mailing list