[PATCH] D49448: [AMDGPU] Fix VGPR spills where offset doesn't fit in 12 bits

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: lib/Target/AMDGPU/SIRegisterInfo.cpp:543
   int64_t Offset = InstOffset + MFI.getObjectOffset(Index);
-  const int64_t OriginalImmOffset = Offset;
+  int64_t ScratchOffsetRegDelta = Offset;
 
----------------
Setting this to Offset does not seem correct as the real value needs to be the scaled offset which is computed below. Here we just need the variable declared and it only has a valid value when RanOutOfSGPRs is true. So perhaps initialize to 0?

Probably a better approach is to eliminate RanOutOfSGPRs and instead just have ScratchOffsetRegDelta which is initialized to 0. Then set it when it is actually used (as already done). Then change the "if (RanOutOfSGPRs)" below to "if (ScratchOffsetRegDelta != 0)".


================
Comment at: lib/Target/AMDGPU/SIRegisterInfo.cpp:548
 
+  const unsigned EltSize = 4;
+
----------------
If this is introduced then seems it should be used as well in the NumSubRegs and Size calculations above which are also in terms of the EltSize.


================
Comment at: lib/Target/AMDGPU/SIRegisterInfo.cpp:550
+
   if (!isUInt<12>(Offset + Size)) {
     SOffset = AMDGPU::NoRegister;
----------------
Should this be Offset + Size - EltSize? Note that the loop is 0 based and increments by EltSize.


Repository:
  rL LLVM

https://reviews.llvm.org/D49448