This is an archive of the discontinued LLVM Phabricator instance.

Don't import variadic functions
ClosedPublic

Authored by Prazek on Aug 9 2016, 5:02 PM.

Details

Summary

This patch adds IsVariadicFunction bit to summary in order
to not import variadic functions. Inliner doesn't inline
variadic functions because it is hard to reason about it.

This one small fix improves Importer by about 16%
(going from 86% to 100% of imported functions that are
inlined anywhere)
on some spec benchmarks like 'int' and others.

Diff Detail

Event Timeline

Prazek updated this revision to Diff 67436.Aug 9 2016, 5:02 PM
Prazek retitled this revision from to Don't import variadic functions.
Prazek updated this object.
Prazek added reviewers: eraman, mehdi_amini, tejohnson.
Prazek added a subscriber: llvm-commits.
mehdi_amini edited edge metadata.Aug 9 2016, 5:07 PM

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

eraman edited edge metadata.Aug 9 2016, 5:23 PM

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

InlineCost.cpp has an isInlineViable function. It doesn't check for vararg now, so that should be first fixed and then can be used to set a bit in the summary.

Prazek added a comment.Aug 9 2016, 5:38 PM

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

InlineCost.cpp has an isInlineViable function. It doesn't check for vararg now, so that should be first fixed and then can be used to set a bit in the summary.

You mean to use isInlineViable to set this bit? It definitely checks more things, but I actually didn't get any of the cases that the functions checks in the SPEC int.
My concern is that this is probably very costly to call.

Prazek added a comment.Aug 9 2016, 5:38 PM

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

Good point Mehdi

Prazek added a comment.Aug 9 2016, 5:43 PM

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

InlineCost.cpp has an isInlineViable function. It doesn't check for vararg now, so that should be first fixed and then can be used to set a bit in the summary.

You mean to use isInlineViable to set this bit? It definitely checks more things, but I actually didn't get any of the cases that the functions checks in the SPEC int.
My concern is that this is probably very costly to call.

I looked on this function, and it seems that inlineCost only calls it to determinate if function marked as "always_inline" should be inlined. I should add if Variadic there, but I don't think we should use this function to determinate the bit.

eraman added a comment.Aug 9 2016, 6:10 PM

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

InlineCost.cpp has an isInlineViable function. It doesn't check for vararg now, so that should be first fixed and then can be used to set a bit in the summary.

You mean to use isInlineViable to set this bit? It definitely checks more things, but I actually didn't get any of the cases that the functions checks in the SPEC int.
My concern is that this is probably very costly to call.

I looked on this function, and it seems that inlineCost only calls it to determinate if function marked as "always_inline" should be inlined. I should add if Variadic there, but I don't think we should use this function to determinate the bit.

What is the benefit of knowing that a function is variadic compared to just knowing that is can't be inlined?
We should just have a bit that is "can't be inlined"

InlineCost.cpp has an isInlineViable function. It doesn't check for vararg now, so that should be first fixed and then can be used to set a bit in the summary.

You mean to use isInlineViable to set this bit? It definitely checks more things, but I actually didn't get any of the cases that the functions checks in the SPEC int.
My concern is that this is probably very costly to call.

I looked on this function, and it seems that inlineCost only calls it to determinate if function marked as "always_inline" should be inlined. I should add if Variadic there, but I don't think we should use this function to determinate the bit.

Yes, I missed the part in getInlineCost where this is called only with the always_inline attribute. All (or most) of the checks in this method is also checked in the regular inliner (SimpleInliner) but only in the reachable path. But it's probably fine not to check for them (and let the inliner reject them later). Even if you're checking for just var_args, this code be added to InlineCost.cpp.

Prazek updated this revision to Diff 67585.Aug 10 2016, 1:09 PM
Prazek edited edge metadata.

changed flag name

mehdi_amini added inline comments.Aug 10 2016, 2:10 PM
lib/Bitcode/Reader/BitcodeReader.cpp
732

Isn't it reversed here? The bit set to one is *not* viable to inline if I read correctly the writer counterpart.
There is something I'm probably missing, otherwise how's the test passing?

Prazek added inline comments.Aug 10 2016, 2:21 PM
lib/Bitcode/Reader/BitcodeReader.cpp
732

good catch, I just forgot to change this name. I firstly wanted to make flag "IsViableToInline", but then because this bit changed from 0 to 1, most tests were invalid.
I talked with Teresa and she said that it will be better to make this flag reverse, so it won't be set in most cases.

mehdi_amini added inline comments.Aug 10 2016, 2:24 PM
lib/Bitcode/Reader/BitcodeReader.cpp
732

So right now the test is *not* passing right? (Probably none of the ThinLTO test should pass with this)

Prazek updated this revision to Diff 67605.Aug 10 2016, 2:27 PM

fixed variable name

Prazek added inline comments.Aug 10 2016, 2:29 PM
lib/Bitcode/Reader/BitcodeReader.cpp
732

no, tests totally pass. This variable name was the only bug here.

Prazek marked 4 inline comments as done.Aug 10 2016, 2:30 PM
mehdi_amini accepted this revision.Aug 10 2016, 2:31 PM
mehdi_amini edited edge metadata.

LGTM, thanks.
We probably need to review other properties of function that prevents them from being inlined.

lib/Bitcode/Reader/BitcodeReader.cpp
732

Oh I see, the ! is in isViableToInline()

This revision is now accepted and ready to land.Aug 10 2016, 2:31 PM
tejohnson accepted this revision.Aug 10 2016, 2:37 PM
tejohnson edited edge metadata.

LGTM with a couple nits.

include/llvm/IR/ModuleSummaryIndex.h
190

Make this also isNotViableToInline, so that we don't negate here and then again at the callsite of this method.

lib/Bitcode/Reader/BitcodeReader.cpp
724

The Version flag is there in case we ever add flags such that older versions of bitcode need to be handled specially. E.g. if you had left this as a positive IsViableToInline, we would probably want to bump the version and set that flag when we had an older version of bitcode (so we didn't completely stop importing when compiling with older bitcode). Since the reversed flag indicates when not to import (and it is a heuristic and not correctness), I don't think there is any need to bump/check the version.

The upshot is that the FIXME can be removed.

LGTM, thanks.
We probably need to review other properties of function that prevents them from being inlined.

This check should be eventually moved to InlineCost so that importer and inliner are out of sync. For now, a FIXME should be useful.

LGTM, thanks.
We probably need to review other properties of function that prevents them from being inlined.

This check should be eventually moved to InlineCost so that importer and inliner are out of sync. For now, a FIXME should be useful.

Very good point indeed.

LGTM, thanks.
We probably need to review other properties of function that prevents them from being inlined.

This check should be eventually moved to InlineCost so that importer and inliner are out of sync. For now, a FIXME should be useful.

Very good point indeed.

So we could check all attributes but they are so not frequent that it wont make huge difference like this one. If the checks are in O(1) then it probably make sense to move them eventually, but things like traversing all instructions to check if function is recursive and other would not make much sense as these functions or instructions are not that frequent (imho)

Prazek marked an inline comment as done.Aug 10 2016, 2:48 PM
Prazek added inline comments.
lib/Bitcode/Reader/BitcodeReader.cpp
724

I see. maybe I should change Version => /*Version, not used right now*/

Prazek updated this revision to Diff 67614.Aug 10 2016, 2:55 PM
Prazek edited edge metadata.

fixes

This revision was automatically updated to reflect the committed changes.