[PATCH] D34471: [Inliner] Boost inlining of an indirect call to always_inline function.

[Xinliang David Li via llvm-commits]

On Thu, Jun 22, 2017 at 9:14 PM, Chandler Carruth via Phabricator <
reviews at reviews.llvm.org> wrote:

> chandlerc added a comment.
>
> In https://reviews.llvm.org/D34471#788736, @davidxl wrote:
>
> > Chandler's example does not involve any indirect calls so it is not
> related to this patch.
>
>
> Actually, it is...
>
> The original test case doesn't look exactly like my example. Rather than a
> template argument and a call through that template argument, it uses an
> indirect call to a normal function argument.
>
> My suggestion is that the code should be changed to use a template
> argument if the intent is to make `always_inline` actually *always* inline.
>
>
There are situations where using template does not apply.

Consider:

1)
  typedef void (*FP) (void);
  const FP  fp[10] = {&foo1, &foo2, ...};  // foo_x marked as always inline

   void bar(int i) {
           fp[i]();
           ...
    }

  void test() {
        bar(0);
   }

2)
 typedef void (*FP) (void);
  const FP  fp[10] = {&foo1, &foo2, ...};

   void bar(int i) {
           fp[i]();
           ...
    }

  void test() {
      for (i = 0; i< 2; i++)      // fully unrolled before inlining
        bar(i);
   }

3)

  struct B {
       virtual void foo();  //marked as always inline
  };

  struct D: public B {
        void foo() override;
   }

   void bar (B* bp) {
         bp->foo();
   }

 void test()  {
    D *dp = new D();
    bar(dp);
 }

..



> > I think Easwaran's patch is consistent with the current inline behavior
> -- it boots inlining to callee with indirect callsites that can be turned
> into direct calls and then further inlined. The amount of boost is the
> benefit of inlining the indirect callsites (i.e., the difference between
> inline cost and allowed threshold). In fact, if the CA.analyzeCall call in
> the existing code
> >
> >   auto IndirectCallParams = Params;
> >   IndirectCallParams.DefaultThreshold = InlineConstants::
> IndirectCallThreshold;
> >   CallAnalyzer CA(TTI, GetAssumptionCache, GetBFI, PSI, *F, CS,
> >                   IndirectCallParams);
> >   if (CA.analyzeCall(CS)) {
> >       ...
> >
> >
> > is turned into getInlineCost(CS), it will achieve similar effect --
> except that InlineCost associated with AlwaysInline is a little too extreme.
> >
> > In many cases, AlwaysInline is very strong hint by the user to indicate
> performance benefit of inlining the function, it does not make sense to
> completely ignore it.
>
> I don't think this is really the right way to model the `always_inline`
> attribute. I continue to think that this attribute should mean that we very
> literally *always* inline. See the discussion here for more details:
> http://lists.llvm.org/pipermail/llvm-dev/2015-August/089466.html


I don't disagree with this.  In reality, always_inline is not used in that
sense literally -- solving that problem (fixing user sources) is a
different problem, especially when the alternative way to suggest a very
strong inline hint does not yet exist.



>
>
> (Note that I *also* am still in favor of having an attribute that is
> actually a very strong hint to do inlining, I just don't think any of the
> existing attributes really fit that bill,


True, and always_inline is used (wrongly) to fit that bill.


> and I don't think the original motivating test case is a particularly
> compelling example. Even if a user *does* want to get really strong
> inlining because of a weird indirect function call, I don't know why the
> inliner has to go to great lengths to detect this rather than expecting the
> user to also attribute the function receiving the function pointer...)
>
>
Firstly, The user/(indirect) caller of the function (marked as always
inline) and the function itself are likely written by different authors.
The library function provider may mark the function with the attribute, and
the client/user may not even aware of the attribute (and the benefit of
inlining it) -- only the compiler/inliner knows about it, so it is not the
job of the client code to mark the caller to be always inline.

Secondly, marking the caller (of an indirect callee with always_inline
attribute) always_inline is usually not the right way to go. The right way
is to mark the callsite as always inline.

Thirdly, the motivation of this patch is not to 'implement' inlining of
 indirect calls to always inline functions, but as an inline heuristics for
better performance.  I guess there will be no objection to the patch if the
attribute checked is not 'always_inline' but the  new very strong inline
hint Chandler suggested.  In other words, this is something inliner needs
to do one way or the other.  We should implement this performance heuristic
for always inline indirect calls now, and when the new attribute is
available, emit warnings about the wrong use of always inline and deprecate
this eventually.

David




> https://reviews.llvm.org/D34471
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170622/97c11c21/attachment.html>