[PATCH] D27344: AMDGPU: Properly implement SIRegisterInfo::isFrameOffsetLegal and needsFrameBaseReg

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIRegisterInfo.cpp:220
 
 bool SIRegisterInfo::needsFrameBaseReg(MachineInstr *MI, int64_t Offset) const {
+  if (!MI->mayLoadOrStore())
----------------
I was working on a patch to change this to just always true (or everything except an add/or instruction) because we can't directly access memory from any instruction. I'm still sort of confused about the relationship between needsFrameBaseReg and isFrameOffsetLegal


================
Comment at: lib/Target/AMDGPU/SIRegisterInfo.cpp:221-233
+  if (!MI->mayLoadOrStore())
+    return false;
+
+  const MachineBasicBlock *MBB = MI->getParent();
+  const MachineFunction *MF = MBB->getParent();
+  const SISubtarget &Subtarget = MF->getSubtarget<SISubtarget>();
+  const SIInstrInfo *TII = Subtarget.getInstrInfo();
----------------
This looks the same as the same as what isFrameOffsetLegal is doing. Can you just call that from here?


================
Comment at: lib/Target/AMDGPU/SIRegisterInfo.cpp:319
+
+  return SIInstrInfo::isMUBUF(*MI) && isUInt<12>(NewOffset);
 }
----------------
I think the isMUBUF check should early exit before all of these other parts


================
Comment at: test/CodeGen/AMDGPU/local-stack-slot-offset.ll:16-17
+main_body:
+  %slot1 = insertelement <513 x float> undef, float %v1, i32 %idx1
+  %slot2 = insertelement <513 x float> undef, float %v2, i32 %idx2
+
----------------
Can you come up with a testcase which doesn't use an enormous illegal vector type? These end up having really bad compile time and I've been meaning to fix the ones we already have 


https://reviews.llvm.org/D27344