[PATCH] D17294: Fix for PR 26500

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:601
@@ +600,3 @@
+  // If we need the second one, initially set it to R0 as well.
+  if (ScratchRegisterCR) {
+    assert(ScratchRegister &&
----------------
Well, there's no guarantee at this point that R12 is available. If I don't do this, we can easily end up in the same situation that prompted the bug. The idea is that I assume we don't have the second scratch register available until one is found. The code that reorders the CR/LR spills depends on the two registers actually being the same.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:616
@@ +615,3 @@
+
+  // Don't consider callee-saved registers
+  for (int i = 0; CSRegs[i]; i++)
----------------
How about:
We shouldn't use callee-saved registers as scratch registers as they may be available when looking for a candidate block for shrink wrapping but not available when the actual prologue/epilogue is being emitted.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:655
@@ +654,3 @@
+  // and the stack frame is large, we need two scratch registers.
+  bool Needs2ScratchRegs = IsLargeFrame && HasBP && MaxAlign > 1;
+
----------------
This is a good point. If I don't have to save the CR but I do have to realign a large stack frame and R0 is available as a scratch register, findScratchRegister will just set ScratchRegisterCR to R12 without checking if it's available.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:661
@@ -612,3 +660,3 @@
 bool PPCFrameLowering::canUseAsEpilogue(const MachineBasicBlock &MBB) const {
   MachineBasicBlock *TmpMBB = const_cast<MachineBasicBlock *>(&MBB);
 
----------------
The code in emitEpilogue only uses the second scratch register for spilling the CR so there's never a requirement for a second scratch register. When emitting the epilogue, we will ask nicely for a second scratch register if we're spilling the CR, but if we don't get the second one, we're still functionally correct (the CR spill sequence completes before the LR sequence starts).

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:753-754
@@ -703,3 +752,4 @@
 
-  findScratchRegister(&MBB, false, &ScratchReg);
+  findScratchRegister(&MBB, false, MustSaveCR, &ScratchReg, &TempReg);
+
   assert(ScratchReg && "No scratch register!");
----------------
Yes, I agree. Well almost. The condition in words should be "if we need to spill the CR OR we need to realign a large stack frame".

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:883
@@ -813,2 +882,3 @@
 
+  // This condition must be kept in sync with canUseAsPrologue.
   if (HasBP && MaxAlign > 1) {
----------------
I see what you mean. It's in sync with canUseAsPrologue but not with the call to findScratchRegister.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:1106-1107
@@ -1033,3 +1105,4 @@
 
-  findScratchRegister(&MBB, true, &ScratchReg);
+  findScratchRegister(&MBB, true, MustSaveCR, &ScratchReg, &TempReg);
+
   assert(ScratchReg && "No scratch register!");
----------------
I think that this would have the potential of causing some functions not to be shrink wrapped unnecessarily. At the very least, it will make blocks that don't have two available scratch registers invalid for epilogue insertion when we technically could use the block as epilogue. This is so because as I mentioned in the comment above, the emitEpilogue function never has a hard requirement for two scratch registers - if we don't have two, we just use the same one for spilling the CR and LR.

So now the question is, do we want that? Using a single register will cause the CR/LR spills to have increased overall latency. On the other hand, if no blocks are available for epilogue insertion, we don't shrink wrap. I can implement it either way.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.h:46
@@ -44,1 +45,3 @@
+   * to be realigned) the function will check for and set two registers.
+   *
    * \param[in] MBB The machine basic block to find an available register for
----------------
I've described that in the return section. Do you want me to add something here as well?


Repository:
  rL LLVM

http://reviews.llvm.org/D17294