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

[pcwalton]

Summary

I've fixed up https://reviews.llvm.org/D9653. Please let me know if you have any further suggestions. Thanks!

[whitequark]

It looks like you inadvertently included an entire copy of D34387 here.

[fhahn]

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.

lib/Target/X86/X86FrameLowering.cpp

71 ↗

(On Diff #103158)

unrelated whitespace change?

657 ↗

(On Diff #103158)

unrelated whitespace change?

801 ↗

(On Diff #103158)

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

838 ↗

(On Diff #103158)

Why aren't you using Is64Bit here? I think you should use the same check as in line 804.

[majnemer]

Please include more context in this diff.

[pcwalton]

lib/Target/X86/X86FrameLowering.cpp

801 ↗

(On Diff #103158)

I did this because StringRef warns that the contents don't have to be null terminated. Is there a guarantee of null termination in this specific instance?

[pcwalton]

Split this out correctly, added more context, and added the missing test.

[majnemer]

docs/LangRef.rst

1478–1480 ↗

(On Diff #103233)

What if both have a probe-stack attribute but they disagree? Should we disallow inlining? Should we ignore the callee attribute?

[pcwalton]

Addressed comments.

[majnemer]

LGTM

[fhahn]

@pcwalton I assume you do not have commit access? In that case somebody else has to commit the change on your behalf and it would be good to let people know.

llvm/trunk/docs/LangRef.rst

* ``"prologue-short-redirect"`` - This style of patchable
        the patchable instruction larger than a short jump.
 
        ``"prologue-short-redirect"`` is currently only supported on
        x86-64.
 
     This attribute by itself does not imply restrictions on
     inter-procedural optimizations.  All of the semantic effects the
     patching may have to be separately conveyed via the linkage type.
+``"probe-stack"``
+    This attribute indicates that the function will trigger a guard region
+    in the end of the stack. It ensures that accesses to the stack must be
+    no further apart than the size of the guard region to a previous
+    access of the stack. It takes one required string value, the name of
+    the stack probing function that will be called.
+
+    If a function that has a ``"probe-stack"`` attribute is inlined into
+    a function with another ``"probe-stack"`` attribute, the resulting
+    function has the ``"probe-stack"`` attribute of the caller. If a
+    function that has a ``"probe-stack"`` attribute is inlined into a
+    function that has no ``"probe-stack"`` attribute at all, the resulting
+    function has the ``"probe-stack"`` attribute of the callee.
 ``readnone``
     On a function, this attribute indicates that the function computes its
     result (or decides to unwind an exception) based strictly on its arguments,
     without dereferencing any pointer arguments or otherwise accessing
     any mutable state (e.g. memory, control registers, etc) visible to
     caller functions. It does not write through any pointer arguments
     (including ``byval`` arguments) and never changes any state visible
     to callers. This means while it cannot unwind exceptions by calling

llvm/trunk/include/llvm/IR/Attributes.td

 
 def : MergeRule<"setAND<LessPreciseFPMADAttr>">;
 def : MergeRule<"setAND<NoInfsFPMathAttr>">;
 def : MergeRule<"setAND<NoNansFPMathAttr>">;
 def : MergeRule<"setAND<UnsafeFPMathAttr>">;
 def : MergeRule<"setOR<NoImplicitFloatAttr>">;
 def : MergeRule<"setOR<NoJumpTablesAttr>">;
 def : MergeRule<"adjustCallerSSPLevel">;
+def : MergeRule<"adjustCallerStackProbes">;

llvm/trunk/lib/IR/Attributes.cpp

if (Callee.hasFnAttribute(Attribute::StackProtectReq)) {
     Caller.removeAttributes(AttributeList::FunctionIndex, OldSSPAttr);
     Caller.addFnAttr(Attribute::StackProtectStrong);
   } else if (Callee.hasFnAttribute(Attribute::StackProtect) &&
              !Caller.hasFnAttribute(Attribute::StackProtectReq) &&
              !Caller.hasFnAttribute(Attribute::StackProtectStrong))
     Caller.addFnAttr(Attribute::StackProtect);
 }
 
+/// \brief If the inlined function required stack probes, then ensure that
+/// the calling function has those too.
+static void adjustCallerStackProbes(Function &Caller, const Function &Callee) {
+  if (!Caller.hasFnAttribute("probe-stack") && Callee.hasFnAttribute("probe-stack"))
+    Caller.addFnAttr("probe-stack", Callee.getFnAttribute("probe-stack").getValueAsString());
+}
+
 #define GET_ATTR_COMPAT_FUNC
 #include "AttributesCompatFunc.inc"
 
 bool AttributeFuncs::areInlineCompatible(const Function &Caller,
                                          const Function &Callee) {
   return hasCompatibleFnAttrs(Caller, Callee);
 }
 
 void AttributeFuncs::mergeAttributesForInlining(Function &Caller,
                                                 const Function &Callee) {
   mergeFnAttrs(Caller, Callee);
 }

llvm/trunk/test/Transforms/Inline/inline-probe-stack.ll

+; RUN: opt %s -inline -S | FileCheck %s
+
+define internal void @inner() "probe-stack"="__probestackinner" {
+  ret void
+}
+
+define void @outerNoAttribute() {
+  call void @inner()
+  ret void
+}
+
+define void @outerConflictingAttribute() "probe-stack"="__probestackouter" {
+  call void @inner()
+  ret void
+}
+
+; CHECK: define void @outerNoAttribute() #0
+; CHECK: define void @outerConflictingAttribute() #1
+; CHECK: attributes #0 = { "probe-stack"="__probestackinner" }
+; CHECK: attributes #1 = { "probe-stack"="__probestackouter" }