patch for functions with indirect calls and always_inline attribute
Eric Christopher
echristo at gmail.com
Mon Apr 14 21:28:30 PDT 2014
On Mon, Apr 14, 2014 at 9:15 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
>> From: "Eric Christopher" <echristo at gmail.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: "llvm-commits" <llvm-commits at cs.uiuc.edu>, "Gerolf Hoflehner" <ghoflehner at apple.com>
>> Sent: Monday, April 14, 2014 11:12:08 PM
>> Subject: Re: patch for functions with indirect calls and always_inline attribute
>>
>> On Mon, Apr 14, 2014 at 9:07 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>> > ----- Original Message -----
>> >> From: "Eric Christopher" <echristo at gmail.com>
>> >> To: "Hal Finkel" <hfinkel at anl.gov>
>> >> Cc: "llvm-commits" <llvm-commits at cs.uiuc.edu>, "Gerolf Hoflehner"
>> >> <ghoflehner at apple.com>
>> >> Sent: Monday, April 14, 2014 11:00:21 PM
>> >> Subject: Re: patch for functions with indirect calls and
>> >> always_inline attribute
>> >>
>> >> On Mon, Apr 14, 2014 at 8:39 PM, Hal Finkel <hfinkel at anl.gov>
>> >> wrote:
>> >> > ----- Original Message -----
>> >> >> From: "Eric Christopher" <echristo at gmail.com>
>> >> >> To: "Gerolf Hoflehner" <ghoflehner at apple.com>
>> >> >> Cc: "llvm-commits" <llvm-commits at cs.uiuc.edu>
>> >> >> Sent: Monday, April 14, 2014 6:11:06 PM
>> >> >> Subject: Re: patch for functions with indirect calls and
>> >> >> always_inline attribute
>> >> >>
>> >> >> On Mon, Apr 14, 2014 at 4:09 PM, Gerolf Hoflehner
>> >> >> <ghoflehner at apple.com> wrote:
>> >> >> > Please see below.
>> >> >> >
>> >> >> > On Apr 14, 2014, at 2:56 PM, Eric Christopher
>> >> >> > <echristo at gmail.com>
>> >> >> > wrote:
>> >> >> >
>> >> >> >> I could be down for this...
>> >> >> >>
>> >> >> >> Do you have any numbers?
>> >> >> > No. I ran into this when creating a test case for another bug
>> >> >> > fix.
>> >> >>
>> >> >> OK, would you mind running some performance numbers on this? It
>> >> >> passes
>> >> >> my sniff test as well, but I'd like to not be wrong :)
>> >> >>
>> >> >> If the numbers come out neutral/ahead/no change feel free to
>> >> >> commit.
>> >> >
>> >> > And if they don't? always_inline means *always inline* -- this
>> >> > is a
>> >> > correctness issue, and we should do it regardless of performance
>> >> > considerations.
>> >> >
>> >>
>> >> Turns out I agree. However, I'd like to know if we've got any
>> >> regressions so they can be explored first and we can figure out
>> >> what's
>> >> wrong and then apply. I wouldn't expect anything, but running
>> >> numbers
>> >> would make sure that we're not exposing some pernicious
>> >> inliner/opt
>> >> bug somewhere else.
>> >
>> > Fair enough.
>> >
>> >>
>> >> > 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.
>> >> >
>> >>
>> >> We do have a bonus to the inliner for just that IIRC. This should
>> >> be
>> >> for something like computed branches within functions rather than
>> >> calls to virtual functions? That said, it does seem odd since the
>> >> branch predictor should help with those as well.
>> >
>> > Good point. So this, in C, is for things like dispatch tables using
>> > the labels-as-values extension; anything else?
>> >
>>
>> I imagine they found this in the webkit interpreter or something like
>> that which uses dispatch tables.
>
> Right, and I suppose they want them inlined ;) -- which makes me wonder whether we should, as a follow-up, benchmark removing the restriction all together.
Yeah, without any comments on the restrictions then it could be
interesting to find out ... and document it either way :)
-eric
>
> -Hal
>
>>
>> -eric
>>
>> > -Hal
>> >
>> >>
>> >> *shrug*
>> >>
>> >> -eric
>> >>
>> >> > -Hal
>> >> >
>> >> >>
>> >> >> Thanks!
>> >> >>
>> >> >> -eric
>> >> >>
>> >> >> >>
>> >> >> >> Nits on the patch:
>> >> >> >>
>> >> >> >> // Disallow inlining of functions which contain an
>> >> >> >> indirect
>> >> >> >> branch.
>> >> >> >> - if (isa<IndirectBrInst>(BI->getTerminator()))
>> >> >> >> + if (isa<IndirectBrInst>(BI->getTerminator()) &&
>> >> >> >> + !F.hasFnAttribute(Attribute::AlwaysInline))
>> >> >> >>
>> >> >> >> Update the comment.
>> >> >> > Ok.
>> >> >> >> I'm assuming this can't be hoisted because of the
>> >> >> >> returns_twice aspect yes?
>> >> >> >
>> >> >> > Yes, for now I only wanted to address indirect branches and
>> >> >> > leave
>> >> >> > remaining checks as they are.
>> >> >> >
>> >> >> >>
>> >> >> >> -define i32 @inner5(i8* %addr) alwaysinline {
>> >> >> >> +define i32 @inner5(i8* %addr) {
>> >> >> >>
>> >> >> >> Why the change?
>> >> >> > This is to keep the old test behavior which relied on *not*
>> >> >> > inlining inner5 because of an indirect branch.
>> >> >> >>
>> >> >> >> -eric
>> >> >> >>
>> >> >> >> On Mon, Apr 14, 2014 at 2:49 PM, Gerolf Hoflehner
>> >> >> >> <ghoflehner at apple.com> wrote:
>> >> >> >>> Hi
>> >> >> >>>
>> >> >> >>> llvm currently does not inline functions with indirect
>> >> >> >>> branches.
>> >> >> >>> We had some
>> >> >> >>> internal discussion and think the always_inline attribute
>> >> >> >>> should
>> >> >> >>> overrule
>> >> >> >>> that restriction.
>> >> >> >>>
>> >> >> >>> Gerolf
>> >> >> >>>
>> >> >> >>>
>> >> >> >>>
>> >> >> >>>
>> >> >> >>>
>> >> >> >>> Index: lib/Analysis/IPA/InlineCost.cpp
>> >> >> >>> ===================================================================
>> >> >> >>> --- lib/Analysis/IPA/InlineCost.cpp (revision 206204)
>> >> >> >>> +++ lib/Analysis/IPA/InlineCost.cpp (working copy)
>> >> >> >>> @@ -1301,7 +1301,8 @@
>> >> >> >>> Attribute::ReturnsTwice);
>> >> >> >>> for (Function::iterator BI = F.begin(), BE = F.end(); BI
>> >> >> >>> !=
>> >> >> >>> BE;
>> >> >> >>> ++BI) {
>> >> >> >>> // Disallow inlining of functions which contain an
>> >> >> >>> indirect
>> >> >> >>> branch.
>> >> >> >>> - if (isa<IndirectBrInst>(BI->getTerminator()))
>> >> >> >>> + if (isa<IndirectBrInst>(BI->getTerminator()) &&
>> >> >> >>> + !F.hasFnAttribute(Attribute::AlwaysInline))
>> >> >> >>> return false;
>> >> >> >>>
>> >> >> >>>
>> >> >> >>>
>> >> >> >>> for (BasicBlock::iterator II = BI->begin(), IE =
>> >> >> >>> BI->end();
>> >> >> >>> II != IE;
>> >> >> >>> Index: test/Transforms/Inline/always-inline-attribute.ll
>> >> >> >>> ===================================================================
>> >> >> >>> --- test/Transforms/Inline/always-inline-attribute.ll
>> >> >> >>> (revision
>> >> >> >>> 0)
>> >> >> >>> +++ test/Transforms/Inline/always-inline-attribute.ll
>> >> >> >>> (working
>> >> >> >>> copy)
>> >> >> >>> @@ -0,0 +1,26 @@
>> >> >> >>> +; RUN: opt < %s -O3 -S | FileCheck %s
>> >> >> >>> + at gv = external global i32
>> >> >> >>> +
>> >> >> >>> +define i32 @main() nounwind {
>> >> >> >>> +; CHECK-NOT: call i32 @foo
>> >> >> >>> + %1 = load i32* @gv, align 4
>> >> >> >>> + %2 = tail call i32 @foo(i32 %1)
>> >> >> >>> + unreachable
>> >> >> >>> +}
>> >> >> >>> +
>> >> >> >>> +define internal i32 @foo(i32) alwaysinline {
>> >> >> >>> + br label %2
>> >> >> >>> +
>> >> >> >>> +; <label>:2 ; preds
>> >> >> >>> =
>> >> >> >>> %8,
>> >> >> >>> %1
>> >> >> >>> + %3 = phi i32 [ %0, %1 ], [ %10, %8 ]
>> >> >> >>> + %4 = phi i8* [ blockaddress(@foo, %2), %1 ], [ %6, %8 ]
>> >> >> >>> + %5 = icmp eq i32 %3, 1
>> >> >> >>> + %6 = select i1 %5, i8* blockaddress(@foo, %8), i8* %4
>> >> >> >>> + %7 = add nsw i32 %3, -1
>> >> >> >>> + br label %8
>> >> >> >>> +
>> >> >> >>> +; <label>:8 ; preds
>> >> >> >>> =
>> >> >> >>> %8,
>> >> >> >>> %2
>> >> >> >>> + %9 = phi i32 [ %7, %2 ], [ %10, %8 ]
>> >> >> >>> + %10 = add nsw i32 %9, -1
>> >> >> >>> + indirectbr i8* %6, [label %2, label %8]
>> >> >> >>> +}
>> >> >> >>> Index: test/Transforms/Inline/always-inline.ll
>> >> >> >>> ===================================================================
>> >> >> >>> --- test/Transforms/Inline/always-inline.ll (revision
>> >> >> >>> 206203)
>> >> >> >>> +++ test/Transforms/Inline/always-inline.ll (working
>> >> >> >>> copy)
>> >> >> >>> @@ -78,7 +78,7 @@
>> >> >> >>> ret i32 %add
>> >> >> >>> }
>> >> >> >>>
>> >> >> >>>
>> >> >> >>>
>> >> >> >>> -define i32 @inner5(i8* %addr) alwaysinline {
>> >> >> >>> +define i32 @inner5(i8* %addr) {
>> >> >> >>> entry:
>> >> >> >>> indirectbr i8* %addr, [ label %one, label %two ]
>> >> >> >>>
>> >> >> >>>
>> >> >> >>> _______________________________________________
>> >> >> >>> llvm-commits mailing list
>> >> >> >>> llvm-commits at cs.uiuc.edu
>> >> >> >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >> >> >>>
>> >> >> >
>> >> >> _______________________________________________
>> >> >> llvm-commits mailing list
>> >> >> llvm-commits at cs.uiuc.edu
>> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >> >>
>> >> >
>> >> > --
>> >> > Hal Finkel
>> >> > Assistant Computational Scientist
>> >> > Leadership Computing Facility
>> >> > Argonne National Laboratory
>> >>
>> >
>> > --
>> > Hal Finkel
>> > Assistant Computational Scientist
>> > Leadership Computing Facility
>> > Argonne National Laboratory
>>
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
More information about the llvm-commits
mailing list