[PATCH] D65923: [X86] Fix stack probe issue on windows32.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Seems reasonable, but I have some suggestions.



================
Comment at: llvm/lib/Target/X86/X86CallFrameOptimization.cpp:247-254
+  bool UseStackProbe =
+    !STI->getTargetLowering()->getStackProbeSymbolName(MF).empty();
+  unsigned StackProbeSize = 4096;
+  const Function &Fn = MF.getFunction();
+  if (Fn.hasFnAttribute("stack-probe-size"))
+    Fn.getFnAttribute("stack-probe-size")
+        .getValueAsString()
----------------
I see this pattern for calculating the stack probe size is already duplicated twice. Please factor it out into a method of X86ISelLowering, perhaps next to getStackProbeSymbolName and update the two callers along with this one.


================
Comment at: llvm/lib/Target/X86/X86CallFrameOptimization.cpp:258-261
+        // If the frame size exceed stack probe size, compiler need
+        // to generate a stack probe call . This pass changes the
+        // reserved stack size and cause of stack probe function not
+        // be inserted, so bypass the pass on this scenario.
----------------
I see that TargetInstrInfo uses the phrase "frame size" to refer to the size of the arguments used to pass call arguments, but I think that's easily confused with the size of the current function's stack frame. Maybe this comment would be clearer:

If any call allocates more argument stack memory than the stack probe size, don't do this optimization. Otherwise, this pass would need to synthesize additional stack probe calls to allocate memory for arguments.


================
Comment at: llvm/lib/Target/X86/X86CallFrameOptimization.cpp:263
+        if (TII->getFrameSize(MI) >= StackProbeSize && UseStackProbe)
+          return false;
         CallContext Context;
----------------
Please move this check into the `isLegal` method where other preconditions are checked.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65923/new/

https://reviews.llvm.org/D65923