-
Notifications
You must be signed in to change notification settings - Fork 486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the inline-threshold flag for setting inliner aggressiveness #6982
base: master
Are you sure you want to change the base?
Conversation
@@ -34,6 +32,7 @@ For each boolean option, you can add a `no-` prefix to switch it off, such as `n | |||
|`dump-uplc`|Bool|False|Dump Untyped Plutus Core| | |||
|`inline-constants`|Bool|True|Always inline constants. Inlining constants always reduces script costs slightly, but may increase script sizes if a large constant is used more than once. Implied by `no-conservative-optimisation`.| | |||
|`inline-fix`|Bool|True|Always inline fixed point combinators. This is generally preferable as it often enables further optimization, though it may increase script size.| | |||
|`inline-threshold`|Int|5|Set inliner aggressiveness. 0 means it avoids making the program bigger. Higher values make the inliner more willing to inline.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, judging on the option name only, I thought that "inline-threshold" is maximum value of some metric, (e.g. maximum program size, or maximum number of terms etc.). This line shows me that its a "level" of inliner aggressiveness.
I propose to name the option either inline-force
or inline-level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: after reading the rest of the code I realize that my initial intuition was right, eventually this value is used as a maximum program increase in Size
that inliner accepts. Its the description that misled me. I also understand that a "maximum program increase in Size
" can be seen as inliner's aggressiveness... indirectly. I mean the description is not "what the option value is" rather "what is an effect of this value".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this should also say that it may increase the program's size by, at maximum, the value provided as the inline-threshold
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventually this value is used as a maximum program increase in Size
As @ana-pantilie said in the other comment, this is not the case. I'll rename it to inline-callsite-growth
, which aligns with the GCC flag inline-unit-growth
.
@@ -20,6 +21,7 @@ data SimplifyOpts name a = SimplifyOpts | |||
, _soConservativeOpts :: Bool | |||
, _soInlineHints :: InlineHints name a | |||
, _soInlineConstants :: Bool | |||
, _soInlineThreshold :: Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use Size
newtype (from the PlutusCore.Size
) because this is what threshold is converted to in the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable.
let -- Inline only if the size is no bigger than not inlining. | ||
sizeIsOk = termSize inlined <= processedTSize | ||
let -- Inline only if the size is no bigger than not inlining plus threshold. | ||
sizeIsOk = termSize inlined <= processedTSize + fromIntegral (max 0 thresh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No fromIntegral
would be needed if thresh has type Size
(from the PlutusCore.Size
module.
@@ -95,6 +95,7 @@ data CompilationOpts a = CompilationOpts { | |||
, _coInlineHints :: InlineHints PLC.Name (Provenance a) | |||
, _coInlineConstants :: Bool | |||
, _coInlineFix :: Bool | |||
, _coInlineThreshold :: Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really ❤️ to use the Size
newtype here.
@@ -76,4 +76,4 @@ prop_inline biVariant = | |||
withMaxSuccess numTestsForPassProp $ | |||
testPassProp | |||
runQuote | |||
$ \tc -> inlinePassSC True tc def (def {_biSemanticsVariant = biVariant}) | |||
$ \tc -> inlinePassSC 0 True tc def (def {_biSemanticsVariant = biVariant}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another benefit that using the Size
newtype gives is that this magic 0 could be less magical:
$ \tc -> inlinePassSC 0 True tc def (def {_biSemanticsVariant = biVariant}) | |
$ \tc -> inlinePassSC (Size 0) True tc def (def {_biSemanticsVariant = biVariant}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it gives good results in practice, or at least on the test cases we have in our repo, but I think that the description makes it sound deceptively safe, see my comments.
...no-constitution/test/Cardano/Constitution/Validator/Data/GoldenTests/sorted.cbor.size.golden
Show resolved
Hide resolved
@@ -34,6 +32,7 @@ For each boolean option, you can add a `no-` prefix to switch it off, such as `n | |||
|`dump-uplc`|Bool|False|Dump Untyped Plutus Core| | |||
|`inline-constants`|Bool|True|Always inline constants. Inlining constants always reduces script costs slightly, but may increase script sizes if a large constant is used more than once. Implied by `no-conservative-optimisation`.| | |||
|`inline-fix`|Bool|True|Always inline fixed point combinators. This is generally preferable as it often enables further optimization, though it may increase script size.| | |||
|`inline-threshold`|Int|5|Set inliner aggressiveness. 0 means it avoids making the program bigger. Higher values make the inliner more willing to inline.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this should also say that it may increase the program's size by, at maximum, the value provided as the inline-threshold
.
-- Inline only if the size is no bigger than not inlining. | ||
sizeIsOk = termSize fullyApplied <= termSize t | ||
-- Inline only if the size is no bigger than not inlining plus threshold. | ||
sizeIsOk = termSize fullyApplied <= termSize t + max 0 (fromIntegral thresh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so the size
is local in the sense that it checks that the size didn't increase for each inline
iteration by more than the amount.
I guess that it could lead to other optimisations being applied, therefore reducing the final size, or even potentially increasing the final size by more than the threshold! I would imagine that the worst case scenario is that the size increases by the threshold amount each iteration, so the final size increases by numIterations * threshold
, this of course assumes that the other optimisations don't increase program size, or do they?
In any case, from the documentation my understanding was that the size threshold is global: after all inlining has been performed the final size of the program is no larger than originalSize + 5
. At least, I think this needs to be clarified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, from the documentation my understanding was that the size threshold is global
I am intentionally not specifying in the documentation how the value of this flag translates to program size! Instead I'm simply saying "Higher values make the inliner more willing to inline". How do you propose modifying the documentation (without making it too verbose)?
I guess that it could lead to other optimisations being applied, therefore reducing the final size
It could also allows to remove the binding. See #6975.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree it's tricky to explain without getting too technical. Either we could say what you just said, that it's not a direct equivalent of program size (but then, what is it?), or we could give a bit more information. Could we just mention that it specifies the maximum amount the program may increase in size per inline iteration? That, together with the idea of calling it as you suggested inline-callsite-growth
should clear up the confusion. If there is slightly more space in the docs, maybe we could also say that it may eventually even decrease program size.
Fixes #6975
inline-threshold = x
means: callsite inlining is allowed to proceed as long as the new size is no more than original size plusx
. So the higherx
is, the more aggressive the inliner inlines. Currently the new size must be no more than the original size (i.e., equivalent tox = 0
), which is too conservative.I experimented with several values between 4 and 20, and found that 5 works the best overall, so I'm setting 5 as the default.