[PATCH] D17294: Fix for PR 26500

[Kit Barton via llvm-commits]

kbarton added inline comments.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:599
@@ -589,1 +598,3 @@
+  // If the two registers are available, we're all good.
+  if (!RS.isRegUsed(R0) && !RS.isRegUsed(R12))
     return true;
----------------
hfinkel wrote:
> It seems like, if we only need one scratch register, and R0 is available, then we can also return true here.
Yes, I agree.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:621
@@ +620,3 @@
+    int FirstScratchReg = BV.find_first();
+    *SR1 = FirstScratchReg == -1 ? R0 : FirstScratchReg;
+  }
----------------
hfinkel wrote:
> Setting this to R0 seems confusing here. First, it is already set to R0 at the beginning of the function, and by definition, R0 is not actually available (right?). It seems better to set it to -1 in this case (or some other invalid value, like 0 (== PPC::NoRegister)) so that we'd catch an accidental use of it later.
R0 is always available because it is the default scratch register. What these checks are essentially doing is determining if we have enough available scratch registers or whether we need to fall back onto the default registers recommended by the ABI.

It's necessary to put the check from line 632 above these checks. 
Once that is done, we can simplify this by just using the bit vector iterators to pull the first one or two free registers from the bit vector. 

================
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;
----------------
nemanjai wrote:
> 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.
This scenario can never happen if this function is used properly. 
As long as it is called exactly the same way (same MBB, same conditions for unique registers) it will return false the first time. 
In canUseAsPrologue, if this returns false, then the specified block cannot be used for the prologue. If you still try to put the prologue there, then you will get incorrect results, but you have not used the functions properly. If you really want to test for this, check the return value for the call emitPrologue/emitEpilogue, and put an assert on that. It should *always* return true from those functions.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:761
@@ -707,1 +760,3 @@
+  bool SingleScratchReg = ScratchReg == TempReg;
+
   int LROffset = getReturnSaveOffset();
----------------
nemanjai wrote:
> 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?
OK, yes, that's true. That assert will provide the same coverage.
Yes, this still requires another review because there are still problems in the findScratchRegisters method. 


Repository:
  rL LLVM

http://reviews.llvm.org/D17294