[PATCH] D34386: [PATCH 1/2] Add a "probe-stack" attribute (revised)

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a subscriber: llvm-commits.
fhahn added a comment.

That's not really my area of expertise, so I only left a few general comments. It seems like you stripped a few trailing whitespaces adding a little bit of noise to the diff. Also, diffs uploaded to Phabricator should contain as much context as possible, to make it easier to see the surrounding code (http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface), e.g. using  `git show HEAD -U999999 > mypatch.patch` and llvm-commits should be added as subscriber when creating a review, so the full history of the review gets mirrored on the mailing list.

I think you should also include a test for your change. You could probably extend `test/CodeGen/X86/stack-probe-size.ll`, by adding an additional RUN line with a triple where stack probes aren't generated at the moment and add  `probe-stack` attributes to some functions.



================
Comment at: lib/Target/X86/X86FrameLowering.cpp:71
 // has any stack objects. However, FI resolution actually has another job,
-// not apparent from the title - it resolves callframesetup/destroy 
+// not apparent from the title - it resolves callframesetup/destroy
 // that were not simplified earlier.
----------------
unrelated whitespace change?


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:657
 
-  // If inlining in the prolog, save RCX and RDX.     
+  // If inlining in the prolog, save RCX and RDX.
   // Future optimization: don't save or restore if not live in.
----------------
unrelated whitespace change?


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:801
 
-  const char *Symbol;
-  if (Is64Bit) {
+  std::string Symbol;
+  if (MF.getFunction()->hasFnAttribute("probe-stack")) {
----------------
Could this be a `const std::string`? Actually I think you could also keep the type `const char *` and use `MF.getFunction()->getFnAttribute("probe-stack").getValueAsString().data()` in line 803, as you are not using any functions from std::string


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:838
 
-  if (Is64Bit) {
+  if (!STI.isTargetWin32()) {
     // MSVC x64's __chkstk and cygwin/mingw's ___chkstk_ms do not adjust %rsp
----------------
Why aren't you using `Is64Bit` here? I think you should use the same check as in line 804.


https://reviews.llvm.org/D34386