patch for functions with indirect calls and always_inline attribute

[Hal Finkel]

----- 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?

 -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