[PATCH] D21551: AMDGPU: fix local stack slot allocation bugs

[Matt Arsenault via llvm-commits]

arsenm added inline comments.

================
Comment at: lib/Target/AMDGPU/SIMachineFunctionInfo.cpp:111-112
@@ -110,4 +108,1 @@
 
-  if (HasStackObjects || MaySpill)
-    PrivateSegmentWaveByteOffset = true;
-
----------------
nhaehnle wrote:
> arsenm wrote:
> > We shouldn't need to always enable this if a new vreg is created for the constant
> Right, but is there a way to tell at this stage? This is the MachineFunctionInfo constructor...
This logic is just if private access is going to be needed. LocalStackSlotAllocation is purely an optimization and doesn't change that. You can create new vregs at that point so you don't need to worry about the special reserved spill registers

================
Comment at: lib/Target/AMDGPU/SIRegisterInfo.cpp:286-290
@@ -285,7 +285,3 @@
 
-  MachineRegisterInfo &MRI = MF->getRegInfo();
-  unsigned UnusedCarry = MRI.createVirtualRegister(&AMDGPU::SReg_64RegClass);
-
-  BuildMI(*MBB, Ins, DL, TII->get(AMDGPU::V_ADD_I32_e64), BaseReg)
-    .addReg(UnusedCarry, RegState::Define | RegState::Dead)
     .addImm(Offset)
----------------
nhaehnle wrote:
> arsenm wrote:
> > This is all pre-RA, so there's no issue creating new virtual registers. A new vreg can be created if the immediate isn't a valid inline immediate for the constant, and then it can be folded/shrunk later if needed.
> Sure, the problem wasn't creating the virtual register, the problem was that the _e64 encoding only allows a very limited set of immediates. That is, the testcase in the diff fails with a "illegal immediate operand" error.
> 
> If some other pass is supposed to pull the immediate out into a V_MOV before the instruction verifier runs, then that wasn't successful...
Yes, but the solution isn't to change the instruction encoding here. You don't need to pick the encoding because the instruction shrinking pass later will reduce it. Whenever possible we pick the e64 form and optimize it down later. You can either just always emit the mov of the constant to a new virtual register with the e64 form (which hopefully SIFoldOperands will take care of when legal), or directly check if it's a legal offset and change the emitted instruction


http://reviews.llvm.org/D21551