[PATCH] D9654: [PATCH 2/2] [x86] Add support for "probe-stack"

[David Majnemer via llvm-commits]

majnemer added a comment.

Can we have a test which exercises the [RE]BX case?


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:205
@@ -204,4 +204,3 @@
 
-    if (Reg == X86::RAX || Reg == X86::EAX || Reg == X86::AX ||
-        Reg == X86::AH || Reg == X86::AL)
+    if (getX86SubSuperRegister(Reg, MVT::i32) == getX86SubSuperRegister(CheckReg, MVT::i32))
       return true;
----------------
`getX86SubSuperRegister(CheckReg, MVT::i32)` can be hoisted out of the loop.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:436-446
@@ +435,13 @@
+
+    if (IsAlive) {
+      auto Reg = getX86SubSuperRegister(RegType, Is64Bit ? MVT::i64 : MVT::i32);
+
+      // Save Reg
+      BuildMI(MBB, MBBI, DL, TII.get(Is64Bit ? X86::PUSH64r : X86::PUSH32r))
+          .addReg(Reg, RegState::Kill)
+          .setMIFlag(MachineInstr::FrameSetup);
+
+      // Reuse the space as a stack allocation
+      NumBytes -= Is64Bit ? 8 : 4;
+    }
+}
----------------
Please make this an early return, it would reduce indentation.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:439
@@ +438,3 @@
+
+      // Save Reg
+      BuildMI(MBB, MBBI, DL, TII.get(Is64Bit ? X86::PUSH64r : X86::PUSH32r))
----------------
Comments should be complete sentences and end with punctuation.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:444
@@ +443,3 @@
+
+      // Reuse the space as a stack allocation
+      NumBytes -= Is64Bit ? 8 : 4;
----------------
Please end this sentence with a period.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:445
@@ +444,3 @@
+      // Reuse the space as a stack allocation
+      NumBytes -= Is64Bit ? 8 : 4;
+    }
----------------
Please use `SlotSize` here.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:456-468
@@ +455,15 @@
+                                                uint64_t &NumBytes) const {
+    if (IsAlive) {
+      // Restore Reg
+      auto Reg = getX86SubSuperRegister(RegType, Is64Bit ? MVT::i64 : MVT::i32);
+
+      auto MIB = BuildMI(MF, DL,
+                         TII.get(Is64Bit ? X86::MOV64rm : X86::MOV32rm),
+                         Reg);
+      MachineInstr *MI = addRegOffset(MIB, StackPtr, false, NumBytes);
+      MI->setFlag(MachineInstr::FrameSetup);
+      MBB.insert(MBBI, MI);
+
+      NumBytes += Is64Bit ? 8 : 4;
+    }
+}
----------------
Please make this an early return, it would reduce indentation.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:876
@@ -820,6 +875,3 @@
 
-    if (isEAXAlive) {
-      // Sanity check that EAX is not livein for this function.
-      // It should not be, so throw an assert.
-      assert(!Is64Bit && "EAX is livein in x64 case!");
+    bool SpillR11 = Is64Bit && MF.getTarget().getCodeModel() == CodeModel::Large;
 
----------------
I think this deserves a comment.

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:881
@@ +880,3 @@
+    pushRegForStackProbeCall(MF, MBB, MBBI, DL, RAXAlive, X86::RAX, NumBytes);
+    pushRegForStackProbeCall(MF, MBB, MBBI, DL, RBXAlive, X86::RBX, NumBytes);
+    if (SpillR11) {
----------------
I think this also deserves a comment.


http://reviews.llvm.org/D9654