[PATCH] Fix hardcoded stack probe space

Philip Reames listmail at philipreames.com
Fri Jan 2 14:34:31 PST 2015


Minor drive by comments.  I'm explicitly deferring giving a LGTM to previous reviewers.

@majnemer -- I would suggest we let this land without addressing your LTO concerns.  While I agree that's an issue, this is a strict improvement over what we have.  If someone files a bug, adding the inliner logic wouldn't be too hard and I'd probably even get to it.


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/X86/X86FrameLowering.cpp:540
@@ -531,1 +539,3 @@
+  }
+
   // If this is x86-64 and the Red Zone is not disabled, if we are a leaf
----------------
Why not use: Fn->hasFnAttribute and Fn->getFnAttribute

================
Comment at: lib/Target/X86/X86Subtarget.cpp:232
@@ +231,3 @@
+  // The page size is 4096 bytes for all targets.
+  static const size_t PageSize = 4096;
+
----------------
Your comment here is unclear.  Specifically, many x86 targets support variable page sizes.  I think what you want here is document the fact that we're *assuming* the page size for a page of stack space is *at least* 4k.    

I'm not sure having this extracted as a named constant adds anything here.

================
Comment at: lib/Target/X86/X86Subtarget.h:301
@@ +300,3 @@
+  /// safely accessed without calling stack probing functions.
+  unsigned getStackProbeSize() const { return stackProbeSize; }
+
----------------
I might name this getDefaultStackProbeSize just to make it clear it's overridable.

http://reviews.llvm.org/D6684

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list