<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Please see below. <div><br><div><div>On Apr 14, 2014, at 8:39 PM, Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 18px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">----- Original Message -----<br><blockquote type="cite">From: "Eric Christopher" <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>><br>To: "Gerolf Hoflehner" <<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>><br>Cc: "llvm-commits" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>><br>Sent: Monday, April 14, 2014 6:11:06 PM<br>Subject: Re: patch for functions with indirect calls and always_inline<span class="Apple-tab-span" style="white-space: pre;"> </span>attribute<br><br>On Mon, Apr 14, 2014 at 4:09 PM, Gerolf Hoflehner<br><<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>> wrote:<br><blockquote type="cite">Please see below.<br><br>On Apr 14, 2014, at 2:56 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>><br>wrote:<br><br><blockquote type="cite">I could be down for this...<br><br>Do you have any numbers?<br></blockquote>No. I ran into this when creating a test case for another bug fix.<br></blockquote><br>OK, would you mind running some performance numbers on this? It<br>passes<br>my sniff test as well, but I'd like to not be wrong :)<br><br>If the numbers come out neutral/ahead/no change feel free to commit.<br></blockquote><br>And if they don't? always_inline means *always inline* -- this is a correctness issue, and we should do it regardless of performance considerations.<br><br>I am curious, however, why we have this restriction at all. Indirect dispatch is expensive, compounding that expense with the call overhead of a small function seems suboptimal; plus, for hardware that has indirect branch prediction (Intel, ARM, and others), inlining should give the branch predictor a better chance at doing the right thing.<br><br>-Hal<br></div></blockquote><div><br></div><div>I see two aspects to it - performance and correctness. Performance modeling of indirect branches is not really attempted yet. With the decision not to inline the cost is implicitly set to infinite. But before performance modeling can be refined correctness must be addressed. I expect a more sophisticated escape/points-to analysis will enable the inliner to decide when it is safe to inline. Without it the user eg. could assign a local block address to a global and jump to that block using that global. If that happens the indirect jump in the inlined routine escapes as a global goto to the original function - which practically is always wrong. In absence of such analysis the safe and cheap strategy is avoiding inlining altogether. Which is perfect from the compiler perspective, and lifting it when the always inline attribute is set now aligns (arguably) better with the user perspective, too.</div><div><br></div><div>-Gerolf</div><div><br></div><br><blockquote type="cite"><div style="font-size: 18px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><blockquote type="cite"><br>Thanks!<br><br>-eric<br><br><blockquote type="cite"><blockquote type="cite"><br>Nits on the patch:<br><br> // Disallow inlining of functions which contain an indirect<br> branch.<br>- if (isa<IndirectBrInst>(BI->getTerminator()))<br>+ if (isa<IndirectBrInst>(BI->getTerminator()) &&<br>+ !F.hasFnAttribute(Attribute::AlwaysInline))<br><br>Update the comment.<br></blockquote>Ok.<br><blockquote type="cite">I'm assuming this can't be hoisted because of the<br>returns_twice aspect yes?<br></blockquote><br>Yes, for now I only wanted to address indirect branches and leave<br>remaining checks as they are.<br><br><blockquote type="cite"><br>-define i32 @inner5(i8* %addr) alwaysinline {<br>+define i32 @inner5(i8* %addr) {<br><br>Why the change?<br></blockquote>This is to keep the old test behavior which relied on *not*<br>inlining inner5 because of an indirect branch.<br><blockquote type="cite"><br>-eric<br><br>On Mon, Apr 14, 2014 at 2:49 PM, Gerolf Hoflehner<br><<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>> wrote:<br><blockquote type="cite">Hi<br><br>llvm currently does not inline functions with indirect branches.<br>We had some<br>internal discussion and think the always_inline attribute should<br>overrule<br>that restriction.<br><br>Gerolf<br><br><br><br><br><br>Index: lib/Analysis/IPA/InlineCost.cpp<br>===================================================================<br>--- lib/Analysis/IPA/InlineCost.cpp (revision 206204)<br>+++ lib/Analysis/IPA/InlineCost.cpp (working copy)<br>@@ -1301,7 +1301,8 @@<br> Attribute::ReturnsTwice);<br> for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE;<br> ++BI) {<br> // Disallow inlining of functions which contain an indirect<br> branch.<br>- if (isa<IndirectBrInst>(BI->getTerminator()))<br>+ if (isa<IndirectBrInst>(BI->getTerminator()) &&<br>+ !F.hasFnAttribute(Attribute::AlwaysInline))<br> return false;<br><br><br><br> for (BasicBlock::iterator II = BI->begin(), IE = BI->end();<br> II != IE;<br>Index: test/Transforms/Inline/always-inline-attribute.ll<br>===================================================================<br>--- test/Transforms/Inline/always-inline-attribute.ll (revision<br>0)<br>+++ test/Transforms/Inline/always-inline-attribute.ll (working<br>copy)<br>@@ -0,0 +1,26 @@<br>+; RUN: opt < %s -O3 -S | FileCheck %s<br>+@gv = external global i32<br>+<br>+define i32 @main() nounwind {<br>+; CHECK-NOT: call i32 @foo<br>+ %1 = load i32* @gv, align 4<br>+ %2 = tail call i32 @foo(i32 %1)<br>+ unreachable<br>+}<br>+<br>+define internal i32 @foo(i32) alwaysinline {<br>+ br label %2<br>+<br>+; <label>:2 ; preds = %8,<br>%1<br>+ %3 = phi i32 [ %0, %1 ], [ %10, %8 ]<br>+ %4 = phi i8* [ blockaddress(@foo, %2), %1 ], [ %6, %8 ]<br>+ %5 = icmp eq i32 %3, 1<br>+ %6 = select i1 %5, i8* blockaddress(@foo, %8), i8* %4<br>+ %7 = add nsw i32 %3, -1<br>+ br label %8<br>+<br>+; <label>:8 ; preds = %8,<br>%2<br>+ %9 = phi i32 [ %7, %2 ], [ %10, %8 ]<br>+ %10 = add nsw i32 %9, -1<br>+ indirectbr i8* %6, [label %2, label %8]<br>+}<br>Index: test/Transforms/Inline/always-inline.ll<br>===================================================================<br>--- test/Transforms/Inline/always-inline.ll (revision 206203)<br>+++ test/Transforms/Inline/always-inline.ll (working copy)<br>@@ -78,7 +78,7 @@<br> ret i32 %add<br>}<br><br><br><br>-define i32 @inner5(i8* %addr) alwaysinline {<br>+define i32 @inner5(i8* %addr) {<br>entry:<br> indirectbr i8* %addr, [ label %one, label %two ]<br><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br><br></blockquote></blockquote><br></blockquote>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br><br></blockquote><br>--<span class="Apple-converted-space"> </span><br>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory</div></blockquote></div><br></div></body></html>