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

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: lib/Target/AMDGPU/SIRegisterInfo.cpp:220
 
 bool SIRegisterInfo::needsFrameBaseReg(MachineInstr *MI, int64_t Offset) const {
+  if (!MI->mayLoadOrStore())
----------------
arsenm wrote:
> 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
ARM does something in isFrameOffsetLegal based on whether the register is SP, but indeed maybe needFrameBaseReg could just be replaced by isFrameOffsetLegal with BaseReg == 0.


================
Comment at: lib/Target/AMDGPU/SIRegisterInfo.cpp:319
+
+  return SIInstrInfo::isMUBUF(*MI) && isUInt<12>(NewOffset);
 }
----------------
arsenm wrote:
> I think the isMUBUF check should early exit before all of these other parts
That's better for consistency, yes. isFrameOffsetLegal currently happens to be called only when needsFrameBaseReg has previously returned true.


================
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
+
----------------
arsenm wrote:
> 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 
Hmm, maybe something with -promote-alloca?


https://reviews.llvm.org/D27344