inference: make Core.Compiler.return_type respect max_methods setting
#46810
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
9232929 to
059345f
Compare
Keno
requested changes
Sep 17, 2022
base/compiler/tfuncs.jl
Outdated
| sv.restrict_abstract_call_sites = old_restrict | ||
| else | ||
| call = abstract_call(interp, ArgInfo(nothing, argtypes_vec), sv, -1) | ||
| call = abstract_call(interp, ArgInfo(nothing, argtypes_vec), sv) |
There was a problem hiding this comment.
This needs to match the implementation of return_type
julia/base/compiler/typeinfer.jl
Line 1142 in 997e336
There was a problem hiding this comment.
Okay, I synced the implementations of Core.Compiler.return_type and return_type_tfunc.
One thing I'm not satisfied with is that the current Core.Compiler.return_type interface doesn't allow it to see caller module context, so it can't respect module-wise max_methods setting.
059345f to
ddcd2f0
Compare
max_methods setting for return_type_tfuncCore.Compiler.return_type respect max_methods setting
Keno
approved these changes
Sep 20, 2022
vtjnash
approved these changes
Nov 28, 2022
190026c to
a48650d
Compare
a48650d to
496192d
Compare
aviatesk
added a commit
that referenced
this pull request
May 12, 2023
Since this part of refactoring is generally useful and I would like to utilize it for other PRs that may be merged before #46810.
aviatesk
added a commit
that referenced
this pull request
May 12, 2023
Since this part of refactoring is generally useful and I would like to utilize it for other PRs that may be merged before #46810.
b349f09 to
9419d6c
Compare
aviatesk
added a commit
that referenced
this pull request
May 13, 2023
This would help inference on `Core.Compiler.return_type(isequal, tt)`
when `tt` is not well inferred (e.g. `tt` is inferred to `Tuple{Any,Any}`).
(although #46810 may disable this `Core.Compiler.return_type`
improvement for good reasons).
Anyway, it is explicitly stated in the documentation that the `isequal`
method should always return a value of `Bool`. So, not only does this
annotation assist inference, it also serves to ensure the correctness of
our code base, and therefore should be beneficial. We may need to take
similar measures for `isless` and `isgreater` (in separate PRs).
aviatesk
added a commit
that referenced
this pull request
May 13, 2023
This would help inference on `Core.Compiler.return_type(isequal, tt)`
when `tt` is not well inferred (e.g. `tt` is inferred to `Tuple{Any,Any}`).
(although #46810 may disable this `Core.Compiler.return_type`
improvement for good reasons).
Anyway, it is explicitly stated in the documentation that the `isequal`
method should always return a value of `Bool`. So, not only does this
annotation assist inference, it also serves to ensure the correctness of
our code base, and therefore should be beneficial. We may need to take
similar measures for `isless` and `isgreater` (in separate PRs).
aviatesk
added a commit
that referenced
this pull request
May 13, 2023
This would help inference on `Core.Compiler.return_type(isequal, tt)`
when `tt` is not well inferred (e.g. `tt` is inferred to `Tuple{Any,Any}`).
(although #46810 may disable this `Core.Compiler.return_type`
improvement for good reasons).
Anyway, it is explicitly stated in the documentation that the `isequal`
method should always return a value of `Bool`. So, not only does this
annotation assist inference, it also serves to ensure the correctness of
our code base, and therefore should be beneficial. We may need to take
similar measures for `isless` and `isgreater` (in separate PRs).
aviatesk
added a commit
that referenced
this pull request
May 16, 2023
This would help inference on `Core.Compiler.return_type(isequal, tt)`
when `tt` is not well inferred (e.g. `tt` is inferred to `Tuple{Any,Any}`).
(although #46810 may disable this `Core.Compiler.return_type`
improvement for good reasons).
Anyway, it is explicitly stated in the documentation that the `isequal`
method should always return a value of `Bool`. So, not only does this
annotation assist inference, it also serves to ensure the correctness of
our code base, and therefore should be beneficial. We may need to take
similar measures for `isless` and `isgreater` (in separate PRs).
9419d6c to
0b975c0
Compare
…ting Previously we didn't limit the number of matching methods for `Core.Compiler.return_type`, which could lead to a potential latency problem. This commit makes `Core.Compiler.return_type` and the corresponding tfunc `return_type_tfunc` respect `max_methods` setting as like the ordinary inference. One caveat here is that the current `Core.Compiler.return_type` interface unfortunately doesn't allow it to see the caller module context, so it can't respect module-wide `max_methods` setting (I guess the change to the interface should be made as of version 2.0), that I think can lead to a potential confusion.
0b975c0 to
48955b4
Compare
|
I am not sure why SparseArrays tests failed so badly (looks like undef arrays being returned?), so I restarted it |
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.
Previously we didn't limit the number of matching methods for
Core.Compiler.return_type, which could lead to a potential latencyproblem.
This commit makes
Core.Compiler.return_typeand the correspondingtfunc
return_type_tfuncrespectmax_methodssetting as like theordinary inference.
One caveat here is that the current
Core.Compiler.return_typeinterfaceunfortunately doesn't allow it to see the caller module context, so it
can't respect module-wide
max_methodssetting (I guess the change tothe interface should be made as of version 2.0), that I think can lead
to a potential confusion.