[PATCH] D17294: Fix for PR 26500

[Nemanja Ivanovic via llvm-commits]

nemanjai 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.
The only reason I don't have that check is that we only know if we absolutely have to have two unique registers. However, when we are in a block that needs to save the CR, we don't absolutely have to have them, but if we do, we emit better code.

I guess the overall idea implemented with this function is this:
- We'll try our best to give you two unique registers
- If we can't, we'll give you one if we can
- If you happen to be asking whether we have enough registers available (1 or 2), we'll let you know

================
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.
I really like the idea of setting it to PPC::NoRegister. After all, if we don't have an available scratch register, it is better to fail than to clobber a live register and have a hard to track-down bug.
The only reason this was setting the register to R0 is to replicate existing semantics.

================
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.
Actually, now that Hal suggested PPC::NoRegister, I actually like that idea best. Then, keeping this check here would have the following outcome:
- If no registers are available, we set both to PPC::NoRegister and return false
- If only one register is available, we set both to that and return true if we only need one, false if we need two
- If two registers are available, we set both accordingly and return true

Finally, I recommend that the calls to this function in emitPrologue/emitEpilogue use the return value of this function in an assert (see below).

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:758
@@ -705,1 +757,3 @@
+                      &TempReg);
+
   assert(ScratchReg && "No scratch register!");
----------------
I think the source of a lot of confusion is that we only use the function here as a mutator and don't check its return value. We ultimately should never be in a situation where this function returns false here. So I recommend having it in an assert:

```
assert(findScratchRegister(&MBB, false, twoScratchRegsRequired(&MBB), &ScratchReg,
       &TempReg) && "Not enough available scratch registers in this block");
```

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:759
@@ -705,1 +758,3 @@
+
   assert(ScratchReg && "No scratch register!");
+  bool SingleScratchReg = ScratchReg == TempReg;
----------------
Actually, I've come to realize that this assert is redundant since findScratchRegister never currently sets the scratch register to zero.


Repository:
  rL LLVM

http://reviews.llvm.org/D17294