Depends on D81224.
Always prefer to clobber input SGPRs and restore them after the
spill. This applies to both spills to VGPRs and scratch.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Can you try to constrain the class of the virtual register earlier to avoid this from happening instead? I think that should avoid this coming up (e.g. we do MRI.constrainRegClass(SrcReg, &AMDGPU::SReg_32_XM0RegClass) already to try avoiding m0 spills, plus in foldMemoryOperandImpl to avoid another problem when spills are optimized out)
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp | ||
---|---|---|
871 | I'm pretty sure this used to be true, so this should depend on the subtarget? |
I could look at that, but it seems advantageous to be able to spill and restore EXEC directly when we at the point of spilling SGPRs?
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp | ||
---|---|---|
871 | If we can establish at what point it became unsupported, but it seems it does not work on anything recent. |
As a reserved register, ppilling of exec should be impossible. The only situations where it "spills" are when a copy was coalesced of another register that spilled. We should be able to satisfy everything by register class constraints. Even if that weren't the case, I would prefer to proceed by first constraining the register classes, and then applying the more complex fix if it turns out there are still cases that can't be avoided
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | ||
---|---|---|
7034–7035 ↗ | (On Diff #271575) | This is now way more complicated. You shouldn't need to loop, or consider specific physical registers. You should be apply to just change the register classes that were used here |
I also think this is missing the test to stress foldMemoryOperandImpl; Can you add one following the example of the test added in d9e0a2942ac71327166a3a597e8383192fd19b17?
- Add test for foldMemoryOperandImpl
- Rework code in foldMemoryOperandImpl to remove references to specific physical registers
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | ||
---|---|---|
7038–7041 ↗ | (On Diff #271959) | This shouldn't consider the register class of the physical register. Only the register class of the virtual register matters (plus getPhysRegClass is really slow), and you shouldn't need to get the size and check if it's an SGPR. Something like: or alternatively |
I'm pretty sure this used to be true, so this should depend on the subtarget?