Page MenuHomePhabricator

[Attributor] IPO across definition boundary of a function marked alwaysinline
ClosedPublic

Authored by bbn on Mar 3 2020, 7:26 PM.

Details

Summary

If a function has alwaysinline attribute and can be inlined, we can treat the function as it has exact definition and perform fix-point analysis on it

Diff Detail

Event Timeline

bbn created this revision.Mar 3 2020, 7:26 PM
Herald added a reviewer: uenoku. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
uenoku added inline comments.Mar 4 2020, 4:40 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
1735

Is FnScope always nonnull here?

bbn marked an inline comment as done.Mar 4 2020, 5:13 AM
bbn added inline comments.
llvm/include/llvm/Transforms/IPO/Attributor.h
1735

I think so, because the outer if has judgement: if(!FnScope || !FnScope->hasExactDefinition())

jdoerfert added inline comments.Mar 4 2020, 8:01 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
1737

If !FnScope is true, FnScope is null. We need to cache the isInlineViable result.

llvm/test/Transforms/Attributor/alwaysinline.ll
79

You need check and check-not lines to make sure we do not derive function attributes where we should not.

bbn updated this revision to Diff 248379.Mar 4 2020, 7:31 PM
bbn marked an inline comment as done.

I have fixed the problem that FnScope may be null and cached the result of isInlineViable. I have also updated my test file to check the attributes.

In D75590#1906851, @bbn wrote:

I have fixed the problem that FnScope may be null and cached the result of isInlineViable. I have also updated my test file to check the attributes.

When I said "cache the result" I meant that we should ensure to call isInlineViable for each function at most once. Maybe at the beginning of the Attributor::run method we can go over all the functions and categorize them. If they have exact linkage *or* are alwaysinline and and viable to be inlined, we say they are "IPO amendable" and we put them in a set. We can then lookup in the set if the function is fine w/o the need to look for all criteria again. Does that make sense?

llvm/test/Transforms/Attributor/alwaysinline.ll
10

What do you mean by "removed safely" here?

22

is this one not readnone?

34

We analyze and annotate it because it is an exact definition, see Function::hasExactDefinition(). Make the function linkonce (or add another pair with linkonce) to see the difference.

bbn updated this revision to Diff 248645.Mar 5 2020, 7:20 PM
  1. Use InlineableFunctions ptr set to track functions that can be inlined
  2. As the ptr set is a private variable for Attributor, so in AA.initialize(), we cannot determine if the inexactly defined function can be inlined or not, so all inexactly defined functions that have alwayinline attribute will keep its state unchanged (just return from the function)
  3. In CreateAAFor function, search the ptr set for the function, if the function has alwayinline attribute but cannot be inlined, we cannot analyze it (indicatePessimisticFixpoint)
bbn marked 4 inline comments as done.Mar 5 2020, 7:29 PM
bbn added inline comments.
llvm/test/Transforms/Attributor/alwaysinline.ll
22

I think it should be readnone, but I tried opt without the patch with the following example:

define void @inner1() alwaysinline {
[...]
}

and the outer1 function does not have the readnone attribute, either.
But when the function does not have alwayinline attribute:

define void @inner1() {
[....]
}

The outer1 function will have the readnone attribute

uenoku added inline comments.Mar 6 2020, 12:33 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
1203

This and all other helpers should be in InformationCache.

1735

style: no brace for one line if-expr

bbn updated this revision to Diff 248708.Mar 6 2020, 5:08 AM

Move InlineableFunctions set to InfoCache

baziotis added inline comments.
llvm/include/llvm/Transforms/IPO/Attributor.h
1113

Nit: You can use !InfoCache.InlineableFunctions.count(FnScope) (it is shorter)

In the initialize method in question, ask the Attributor if a function is "IPO amendable". We need a new helper method for that in the Attributor. It will look at the position, linkage (via the isexact definition), and alwaysinline + inlinable "attributes". This way it we have a single location to do the lookup. It is reusable and easier to understand.

llvm/lib/Transforms/IPO/Attributor.cpp
8297

Only check this if the alwaysinline attribute is set, it is not free and otherwise not needed.

llvm/test/Transforms/Attributor/alwaysinline.ll
22

I'm confused. Take a look at https://godbolt.org/z/EsQWyN . without the linkonce we determine readnone in either case.

bbn updated this revision to Diff 248887.Mar 6 2020, 8:44 PM

Created a helper function in Attributor which determines whether the function is IPO amendable and replaced hasExactDefinition with this helper function.

jdoerfert accepted this revision.Mar 6 2020, 10:44 PM

I think this is all good, two minor comments below. Once fixed I commit it

llvm/include/llvm/Transforms/IPO/Attributor.h
641

make it const function, and appropriate below.

804

Nit: return InfoCache.InlineableFunctions.count(&F) (no braces)
or even:
return F.hasExactDefinition() || InfoCache.InlineableFunctions.count(&F);

This revision is now accepted and ready to land.Mar 6 2020, 10:44 PM
bbn updated this revision to Diff 248904.Mar 6 2020, 11:20 PM
  • Added const for InlineableFunctions
  • updated isFunctionIPOAmendable()
This revision was automatically updated to reflect the committed changes.