Skip to content
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

effects: add reflection utility for the new effect analysis #44785

Merged
merged 3 commits into from
Mar 31, 2022

Conversation

aviatesk
Copy link
Member

This commit adds new reflection utility named Base.infer_effects that
works in the same way as Base.return_types but returns inferred effects
instead. It would be helpful to test that certain method call has an
expected effects.

For example, we can now remove Base.@pure annotation from the definition of
BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}
and checks it's still eligible for concrete evaluation like this
(see #44776 (comment) for the context):

julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown

julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}
           if Base.typename(A) === Base.typename(B)
               return A(Val(max(M, N)))
           end
           return Unknown()
       end
BroadcastStyle (generic function with 1 method)

julia> # test that the above definition is eligible for concrete evaluation
       @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error
Test Passed

@aviatesk aviatesk requested review from vtjnash and Keno March 29, 2022 06:52
base/compiler/types.jl Outdated Show resolved Hide resolved
@aviatesk aviatesk force-pushed the avi/infer_effects branch 2 times, most recently from d95188a to fbfec6c Compare March 29, 2022 07:35
@aviatesk
Copy link
Member Author

I'd like to merge #44786 first before merging this PR.

@aviatesk aviatesk force-pushed the avi/infer_effects branch 3 times, most recently from 082c123 to d744e44 Compare March 30, 2022 11:58
@Keno Keno requested a review from tkf March 30, 2022 20:03
@aviatesk
Copy link
Member Author

Note: I will add Core.Compiler.return_type alternative for this in this PR (or in another PR). It will come with tfunc and similar supports so that we can fold inferred effects at compile time when possible (albeit it can be unreliable as return_type).

@aviatesk aviatesk added the backport 1.8 Change should be backported to release-1.8 label Mar 31, 2022
@aviatesk aviatesk force-pushed the avi/infer_effects branch from d744e44 to 5146e35 Compare March 31, 2022 02:44
@aviatesk
Copy link
Member Author

Ok, I'd like to move this forward quickly if possible, and let me work on Core.Compiler.infer_effects in another PR. @tkf can I ask your review on this?

base/reflection.jl Outdated Show resolved Hide resolved
This commit adds new reflection utility named `Base.infer_effects` that
works in the same way as `Base.return_types` but returns inferred effects
instead. It would be helpful to test that certain method call has an
expected effects.

For example, we can now remove `Base.@pure` annotation from the definition of
`BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}`
and checks it's still eligible for concrete evaluation like this
(see <#44776 (comment)> for the context):
```julia
julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown

julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}
           if Base.typename(A) === Base.typename(B)
               return A(Val(max(M, N)))
           end
           return Unknown()
       end
BroadcastStyle (generic function with 1 method)

julia> # test that the above definition is eligible for concrete evaluation
       @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error
Test Passed
```
@aviatesk aviatesk force-pushed the avi/infer_effects branch from 5146e35 to 51f4632 Compare March 31, 2022 03:10
Comment on lines 1325 to 1329
if isempty(matches)
# although this call is known to throw MethodError (thus `nothrow=ALWAYS_TRUE`),
# still mark it `TRISTATE_UNKNOWN` just in order to be consistent with a result
# derived by the effect analysis, which can't prove guaranteed throwness at this moment
return Core.Compiler.Effects(effects; nothrow=Core.Compiler.TRISTATE_UNKNOWN)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not too clear what effects we should return for this case.
For now, I made the result equivalent to the following case:

julia> function f_no_methods end
f_no_methods (generic function with 0 methods)

julia> Base.infer_effects() do
           throw(MethodError(f_no_methods, ()))
       end
(+c,+e,?n,+t)

base/reflection.jl Outdated Show resolved Hide resolved
Co-authored-by: Takafumi Arakaki <[email protected]>
Copy link
Member

@tkf tkf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't review all the details but it LGTM! The API is what I wanted.

base/compiler/types.jl Outdated Show resolved Hide resolved
@aviatesk aviatesk merged commit 82ce311 into master Mar 31, 2022
@aviatesk aviatesk deleted the avi/infer_effects branch March 31, 2022 07:28
aviatesk added a commit that referenced this pull request Mar 31, 2022
This commit adds new reflection utility named `Base.infer_effects` that
works in the same way as `Base.return_types` but returns inferred effects
instead. It would be helpful to test that certain method call has an
expected effects.

For example, we can now remove `Base.@pure` annotation from the definition of
`BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}`
and checks it's still eligible for concrete evaluation like this
(see <#44776 (comment)> for the context):
```julia
julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown

julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}
           if Base.typename(A) === Base.typename(B)
               return A(Val(max(M, N)))
           end
           return Unknown()
       end
BroadcastStyle (generic function with 1 method)

julia> # test that the above definition is eligible for concrete evaluation
       @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error
Test Passed
```

Co-authored-by: Takafumi Arakaki <[email protected]>
@aviatesk aviatesk mentioned this pull request Mar 31, 2022
67 tasks
aviatesk added a commit that referenced this pull request Mar 31, 2022
This commit adds new reflection utility named `Base.infer_effects` that
works in the same way as `Base.return_types` but returns inferred effects
instead. It would be helpful to test that certain method call has an
expected effects.

For example, we can now remove `Base.@pure` annotation from the definition of
`BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}`
and checks it's still eligible for concrete evaluation like this
(see <#44776 (comment)> for the context):
```julia
julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown

julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}
           if Base.typename(A) === Base.typename(B)
               return A(Val(max(M, N)))
           end
           return Unknown()
       end
BroadcastStyle (generic function with 1 method)

julia> # test that the above definition is eligible for concrete evaluation
       @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error
Test Passed
```

Co-authored-by: Takafumi Arakaki <[email protected]>
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants