[PATCH] D45524: Fix incorrect choice of callee-saved registers save/restore points

[Momchil Velikov via Phabricator via llvm-commits]

chill marked 3 inline comments as done.
chill added inline comments.


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:262
+  // for save/restore placement even if they use SP.
+  if (MI.isCall() && MI.isReturn())
+    return false;
----------------
thegameg wrote:
> I think this is incorrect if the instruction calls a function with a different calling convention, right? In that case some CSRs would be clobbered while otherwise they were expected to be preserved.
> 
> IIUC, the issue here is that some instructions are marked as `let Uses = [SP]`, like calls and returns, but are safe to skip during the analysis.
> 
> If I'm not missing anything, to handle all the cases I think it's better to verify all the register operands and reg masks, then only skip the instruction if it only uses SP and `isCall` or something similar. I'm not sure if `isCall` would handle all the cases, but from a quick glance over *InstrInfo.td, other instructions that have `Uses = [SP]` should definitely affect the placement of the save/restore blocks.
Thanks, indeed. For register masks, if I understand correctly they are used with call instructions and these should preserve SP, so no need to actually check it ?


================
Comment at: lib/CodeGen/ShrinkWrap.cpp:265
+
+  const TargetLowering *TLI = MI.getMF()->getSubtarget().getTargetLowering();
   for (const MachineOperand &MO : MI.operands()) {
----------------
junbuml wrote:
> Why don't we make it as a member variable? 
Right, I made the stack pointer register number a member variable.


https://reviews.llvm.org/D45524