[PATCH] D17294: Fix for PR 26500

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:615
@@ +614,3 @@
+  // were added as live-in to the prologue block by PrologueEpilogueInserter.
+  for (int i = 0; CSRegs[i]; i++)
+    BV.reset(CSRegs[i]);
----------------
kbarton wrote:
> use unsigned and ++i, not i++. 
I don't see why this would make any difference for a primitive type, but I will certainly change it.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:632
@@ +631,3 @@
+  // whether we were unable to provide enough.
+  if (BV.empty() || (BV.count() < 2 && TwoUniqueRegsRequired))
+    return false;
----------------
kbarton wrote:
> You need to move this check above line 618. If there are not two unique registers, you need to return false and set SR1 and SR2 to the default values. This will return false and set the scratch registers to be the same.
That seems like a very bad idea to me. What the result would be is that we set the SR1 and SR2 to R0 and R12 respectively knowing that these registers are not available. Don't forget that in emitPrologue/emitEpilogue we do not use the return value at all - we just use the function to get the registers that are available.
If we somehow end up in a situation where we are inserting the prologue/epilogue into a block that doesn't have the R0/R12 available, the function will happily go on it's merry way using the two registers.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:643
@@ +642,3 @@
+// and the stack frame is large, we need two scratch registers.
+bool PPCFrameLowering::twoScratchRegsRequired(MachineBasicBlock *MBB) const {
+  const PPCRegisterInfo *RegInfo =
----------------
kbarton wrote:
> Please use the term unique here. This isn't about getting two registers - it's about getting unique registers. 
OK.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:761
@@ -707,1 +760,3 @@
+  bool SingleScratchReg = ScratchReg == TempReg;
+
   int LROffset = getReturnSaveOffset();
----------------
kbarton wrote:
> please add:
> assert(!(SingleScratchReg && twoScratchRegsRequired) && "I asked for two unique registers!);
> 
> This should assure that if you asked for two unique registers, you really got two unique registers.
OK, will do. I only didn't add it here because I have what is essentially an equivalent assert on line 905. Do you think this requires another review?

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.h:53
@@ +52,3 @@
+   * \param[in] TwoUniqueRegsRequired Specify whether this basic block will
+   *                                  require two scratch registers.
+   * \param[out] SR1 The scratch register to use
----------------
kbarton wrote:
> ... require two unique scratch registers.
OK.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.h:62
@@ -51,1 +61,3 @@
+   *         false if the required number of scratch register were not available
+   *         and R0 is being used as the default.
    */
----------------
kbarton wrote:
> ... were not available and the defaults are being used.
Yeah, makes sense. I'll change it.


Repository:
  rL LLVM

http://reviews.llvm.org/D17294