[PATCH] D20263: X86: Avoid using _chkstk when lowering WIN_ALLOCA instructions

[Reid Kleckner via llvm-commits]

rnk added inline comments.

================
Comment at: include/llvm/CodeGen/MachineFrameInfo.h:282
@@ +281,3 @@
+  /// Whether the function has WIN_ALLOCA instructions.
+  bool HasWinAlloca = false;
+
----------------
I think WIN_ALLOCA is x86-specific, so a better place for this would be lib/Target/X86/X86MachineFunctionInfo.h

================
Comment at: lib/Target/X86/X86.h:62
@@ -61,1 +61,3 @@
 
+/// XXX
+FunctionPass *createX86WinAllocaExpander();
----------------
mkuper wrote:
> I think this needs a description. :-)
write the comment :)

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:16606
@@ -16609,1 +16605,3 @@
+    unsigned SizeReg = MRI.createVirtualRegister(getRegClassFor(SPTy));
+    Chain = DAG.getCopyToReg(Chain, dl, SizeReg, Size);
     SDVTList NodeTys = DAG.getVTList(MVT::Other, MVT::Glue);
----------------
mkuper wrote:
> Do we still need a CopyToReg? The previous code had it because it needed specifically a copy to EAX glued to the future-chkstk. Can you use Size directly?
I wonder if we could do something clever to avoid creating a copy of an immediate. Might be too much work.

================
Comment at: lib/Target/X86/X86WinAllocaExpander.cpp:116
@@ +115,3 @@
+  // Do a one-pass reverse post-order walk of the CFG to conservatively estimate
+  // the offset between the stack pointer and the lowerst touched part of the
+  // stack, and use that to decide how to lower each WinAlloca instruction.
----------------
s/lowerst/lowest/

================
Comment at: lib/Target/X86/X86WinAllocaExpander.cpp:119
@@ +118,3 @@
+
+  // Compute the reverse post-order.
+  SmallVector<MachineBasicBlock*, 16> RPO;
----------------
This can be:
  ReversePostOrderTraversal<MachineFunction *> RPOT(&MF);

================
Comment at: lib/Target/X86/X86WinAllocaExpander.cpp:168
@@ +167,3 @@
+          MI.getOperand(0).getReg() == StackPtr)
+        Offset = -1;
+    }
----------------
Should this be INT32_MAX instead? It seems like we could do alloca, save+restore, alloca and run over the guard page this way. Maybe a good test case?

================
Comment at: lib/Target/X86/X86WinAllocaExpander.cpp:197
@@ +196,3 @@
+  if (Amount != -1) {
+    assert((Amount % (Is64Bit ? 8 : 4) == 0) && "Stack would not be aligned!");
+  }
----------------
Maybe cache `Is64Bit ? 8 : 4` as a `SlotSize` member variable. Also RAX as... some variable. RegA?


http://reviews.llvm.org/D20263