Stop setting force_noinline for functions of union arguments #27057
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
Didn't something like this get merged? |
|
Did this actually get merged? I don't remember that happening. |
|
oh, apparently not. why do we have that check in there? |
|
bump |
1 similar comment
|
bump |
|
bump? |
Keno
force-pushed
the
kf/unionpenalties
branch
2 times, most recently
from
89afe85
to
009ffe2
Compare
Keno
force-pushed
the
kf/unionpenalties
branch
from
009ffe2
to
9c75dfa
Compare
The original motivation for setting force_noinline for functions with union arguments seems to have been bugs in type intersection. However, the type system has improved quite a bit and at least in the test suites we don't seem to run into any issues. Thus, we can stop setting force_noinline. Nevertheless, it still make since to penalize functions with union arguments. Doing so encourages union splits to happen at the call site, rather than having multiple redundant union splits inside the function. However, for simple functions of intrinsics and builtins it can make a lot of sense to inline the union split signature.
Keno
force-pushed
the
kf/unionpenalties
branch
from
9c75dfa
to
fa4d0d7
Compare
Keno
added a commit
that referenced
this pull request
Jul 5, 2023
I added this code back in #27057, when I first made Union-full signatures inlineable. The justification was to try to encourage the union splitting to happen on the outside. However (and I believe this changed since this code was introduced), these days inference is in complete control of union splitting and we do not take inlineability or non-inlineability of the non-unionsplit function into account when deciding how to inline. As a result, the only effect of the union split penalties was to prevent inlining of functions that are not union-split eligible (e.g. `+(::Vararg{Union{Int, Missing}, 3})`), but are nevertheless cheap by our inlining metric. There is really no reason not to try to inline such functions, so delete this logic.
Keno
added a commit
that referenced
this pull request
Jul 6, 2023
I added this code back in #27057, when I first made Union-full signatures inlineable. The justification was to try to encourage the union splitting to happen on the outside. However (and I believe this changed since this code was introduced), these days inference is in complete control of union splitting and we do not take inlineability or non-inlineability of the non-unionsplit function into account when deciding how to inline. As a result, the only effect of the union split penalties was to prevent inlining of functions that are not union-split eligible (e.g. `+(::Vararg{Union{Int, Missing}, 3})`), but are nevertheless cheap by our inlining metric. There is really no reason not to try to inline such functions, so delete this logic.
Keno
added a commit
that referenced
this pull request
Jul 6, 2023
I added this code back in #27057, when I first made Union-full signatures inlineable. The justification was to try to encourage the union splitting to happen on the outside. However (and I believe this changed since this code was introduced), these days inference is in complete control of union splitting and we do not take inlineability or non-inlineability of the non-unionsplit function into account when deciding how to inline. As a result, the only effect of the union split penalties was to prevent inlining of functions that are not union-split eligible (e.g. `+(::Vararg{Union{Int, Missing}, 3})`), but are nevertheless cheap by our inlining metric. There is really no reason not to try to inline such functions, so delete this logic.
Keno
added a commit
that referenced
this pull request
Jul 7, 2023
I added this code back in #27057, when I first made Union-full signatures inlineable. The justification was to try to encourage the union splitting to happen on the outside. However (and I believe this changed since this code was introduced), these days inference is in complete control of union splitting and we do not take inlineability or non-inlineability of the non-unionsplit function into account when deciding how to inline. As a result, the only effect of the union split penalties was to prevent inlining of functions that are not union-split eligible (e.g. `+(::Vararg{Union{Int, Missing}, 3})`), but are nevertheless cheap by our inlining metric. There is really no reason not to try to inline such functions, so delete this logic.
Keno
added a commit
that referenced
this pull request
Jul 7, 2023
I added this code back in #27057, when I first made Union-full signatures inlineable. The justification was to try to encourage the union splitting to happen on the outside. However (and I believe this changed since this code was introduced), these days inference is in complete control of union splitting and we do not take inlineability or non-inlineability of the non-unionsplit function into account when deciding how to inline. As a result, the only effect of the union split penalties was to prevent inlining of functions that are not union-split eligible (e.g. `+(::Vararg{Union{Int, Missing}, 3})`), but are nevertheless cheap by our inlining metric. There is really no reason not to try to inline such functions, so delete this logic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original motivation for setting force_noinline for functions
with union arguments seems to have been bugs in type intersection.
However, the type system has improved quite a bit and at least in
the test suites we don't seem to run into any issues. Thus, we can
stop setting force_noinline. Nevertheless, it still make since
to penalize functions with union arguments. Doing so encourages
union splits to happen at the call site, rather than having multiple
redundant union splits inside the function. However, for simple functions
of intrinsics and builtins it can make a lot of sense to inline
the union split signature.