[PATCH] D17294: Fix for PR 26500

[Hal Finkel via llvm-commits]

hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

A few comments, but otherwise, LGTM. Thanks!


================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:633
@@ +632,3 @@
+  unsigned FrameSize = determineFrameLayout(MF);
+  bool isLargeFrame = !isInt<16>(-FrameSize);
+  MachineFrameInfo *MFI = MF.getFrameInfo();
----------------
Variable names start with a capital letter. (I see it is like this below also, so please fix the others in a follow-up commit).

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:637
@@ +636,3 @@
+
+  bool Needs2ScratchRegs = isLargeFrame && HasBP && MaxAlign > 1;
+
----------------
We need a comment somewhere explaining what's going on, and this might be a good place.

  // We need a scratch register for spilling LR and for spilling CR. By default, we use two scratch registers to hide latency. However, if only one scratch register is available, we can adjust for that by not overlapping the spill code. However, if we need to realign the stack (i.e. have a base pointer) and the stack frame is large, we need two scratch registers.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.cpp:867
@@ -813,3 +866,3 @@
 
   if (HasBP && MaxAlign > 1) {
     if (isPPC64)
----------------
Please add a comment here noting that this condition needs to be kept in sync with the check in canUseAsPrologue.

================
Comment at: lib/Target/PowerPC/PPCFrameLowering.h:56
@@ -55,1 +55,3 @@
+                           unsigned *ScratchRegisterCR,
+                           bool Needs2ScratchRegs) const;
 
----------------
I think that the code would be easier to read if you made some of these have default parameters:

  bool findScratchRegister(MachineBasicBlock *MBB,
                             bool UseAtEnd,
                             bool Needs2ScratchRegs = false,
                             unsigned *ScratchRegister = nullptr,
                             unsigned *ScratchRegisterCR = nullptr) const;

and then adjusted the call sites accordingly.



Repository:
  rL LLVM

http://reviews.llvm.org/D17294