inference: always bail out const-prop' with non-const result limited #52836
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
aviatesk
added
REPL
|
LGTM |
aviatesk
force-pushed
the
avi/52763
branch
2 times, most recently
from
bd2a136 to
53e69eb
Compare
aviatesk
commented
Jan 11, 2024
test/compiler/inference.jl
Outdated
Comment on lines
4940
to
4954
| @newinterp RecurseInterpreter | ||
| function CC.bail_out_const_call(interp::RecurseInterpreter, result::CC.MethodCallResult, | ||
| si::CC.StmtInfo, sv::CC.AbsIntState) | ||
| if result.rt isa CC.LimitedAccuracy | ||
| return false # allow constprop to recurse into unresolved cycles | ||
| end | ||
| return @invoke CC.bail_out_const_call(interp::CC.AbstractInterpreter, result::CC.MethodCallResult, | ||
| si::CC.StmtInfo, sv::CC.AbsIntState) | ||
| end | ||
| Base.@constprop :aggressive type_level_recurse1(x...) = x[1] == 2 ? 1 : (length(x) > 100 ? x : type_level_recurse2(x[1] + 1, x..., x...)) | ||
| Base.@constprop :aggressive type_level_recurse2(x...) = type_level_recurse1(x...) | ||
| type_level_recurse_entry() = Val{type_level_recurse1(1)}() | ||
| @test Base.return_types(type_level_recurse_entry, ()) |> only == Val{1} | ||
| @test Base.infer_return_type(type_level_recurse_entry, (); interp=RecurseInterpreter()) == Val{1} |
There was a problem hiding this comment.
@Keno This PR prevents const-prop' in the presence of unresolvable cycles, regardless of whether const-prop' is enforced. Consequently, it causes the test cases from #48045 to fail. For interpreters requiring optimization against heavy recursions, like Diffractor, do you think it would be fine to make them add explicit overloads to permit const-prop' for cycles as like RecurseInterpreter here?
There was a problem hiding this comment.
I don't fully know. I'm now entirely happy with the recursion story with respect to Diffractor anyway, so I think that needs a larger solution. As long as this doesn't end up causing us immediate problems, I think it's fine.
There was a problem hiding this comment.
In that case, I'll move forward with this plan. I'll implement the necessary adjustments in Diffractor that would be similar to what RecurseInterpreter does here.
|
@nanosoldier |
aviatesk
force-pushed
the
avi/52763
branch
4 times, most recently
from
20747a9 to
a9d2126
Compare
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
The package evaluation job you requested has completed - possible new issues were detected. |
aviatesk
force-pushed
the
avi/52763
branch
2 times, most recently
from
ef08e37 to
b0be24e
Compare
Investigating into #52763, I've found that `AbstractInterpreters` which enables the `aggressive_constprop` option, such as `REPLInterpreter`, might perform const-prop' even if the result of a non-const call is `LimitedAccuracy'. This can lead to an unintended infinite loop with a custom aggressive const-prop' implementation. This commit restricts const-prop' for such cases where the non-const call result is limited to avoid the issue. This fix is conservative, but given that accurate inference is mostly impossible when there are unresolvable cycles (which is indicated by limited result), aggressive const-prop' isn't necessary for such cases, and I don't anticipate this leading to any obvious regression. fix #52763
aviatesk
added a commit
that referenced
this pull request
Jan 16, 2024
…52836) Investigating into #52763, I've found that `AbstractInterpreters` which enables the `aggressive_constprop` option, such as `REPLInterpreter`, might perform const-prop' even if the result of a non-const call is `LimitedAccuracy`. This can lead to an unintended infinite loop with a custom aggressive const-prop' implementation. This commit restricts const-prop' for such cases where the non-const call result is limited to avoid the issue. This fix is conservative, but given that accurate inference is mostly impossible when there are unresolvable cycles (which is indicated by limited result), aggressive const-prop' isn't necessary for such cases, and I don't anticipate this leading to any obvious regression. fix #52763
Drvi
pushed a commit
to RelationalAI/julia
that referenced
this pull request
Jun 7, 2024
…uliaLang#52836) Investigating into JuliaLang#52763, I've found that `AbstractInterpreters` which enables the `aggressive_constprop` option, such as `REPLInterpreter`, might perform const-prop' even if the result of a non-const call is `LimitedAccuracy`. This can lead to an unintended infinite loop with a custom aggressive const-prop' implementation. This commit restricts const-prop' for such cases where the non-const call result is limited to avoid the issue. This fix is conservative, but given that accurate inference is mostly impossible when there are unresolvable cycles (which is indicated by limited result), aggressive const-prop' isn't necessary for such cases, and I don't anticipate this leading to any obvious regression. fix JuliaLang#52763
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.
Investigating into #52763, I've found that
AbstractInterpreterswhich enables theaggressive_constpropoption, such asREPLInterpreter, might perform const-prop' even if the result of a non-const call isLimitedAccuracy. This can lead to an unintended infinite loop with a custom aggressive const-prop' implementation.This commit restricts const-prop' for such cases where the non-const call result is limited to avoid the issue. This fix is conservative, but given that accurate inference is mostly impossible when there are unresolvable cycles (which is indicated by limited result), aggressive const-prop' isn't necessary for such cases, and I don't anticipate this leading to any obvious regression.
fix #52763