[PATCH] D17294: Fix for PR 26500

[Hal Finkel via llvm-commits]

hfinkel added a comment.

Some additional comments below. As far as I'm concerned, you can commit and we can do additional fixups as followup.


================
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;
----------------
It seems like, if we only need one scratch register, and R0 is available, then we can also return true here.

================
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]);
----------------
nemanjai wrote:
> 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.
ints are preferred (because the compiler can know they can't wrap), so we tend to only use unsiged when necessary for some reason (although we're not entirely consistent about it). ++i is preferred because we only use post-increment if there's a reason (because it can be more expensive for non-primitive types, and no one should have to think about it otherwise).

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:621
@@ +620,3 @@
+    int FirstScratchReg = BV.find_first();
+    *SR1 = FirstScratchReg == -1 ? R0 : FirstScratchReg;
+  }
----------------
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D17294