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: handle va-method properly when refining :inaccessiblememonly #48854

Merged
merged 1 commit into from
Mar 3, 2023

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Mar 2, 2023

Previously the :inaccessiblememonly effect bit may be wrongly refined when analyzing va-method, e.g.:

julia> callgetfield1(xs...) = getfield(getfield(xs, 1), 1)
callgetfield1 (generic function with 1 method)

julia> Base.infer_effects(callgetfield1, (Base.RefValue{Symbol},))
(+c,+e,!n,+t,+s,+m,+i) # inaccessiblememonly is wrongly refined here

This leads to wrong concrete evaluation of callgetfield1 and thus may result in a problem like below:

julia> const GLOBAL_XS = Ref(:julia);

julia> global_getfield() = callgetfield1(GLOBAL_XS);

julia> @test let
           Base.Experimental.@force_compile
           global_getfield()
       end === :julia
Test Passed

julia> GLOBAL_XS[] = :julia2;

julia> @test let
           Base.Experimental.@force_compile
           global_getfield()
       end === :julia2 # this fails
Test Failed at REPL[8]:1
  Expression: let
        #= REPL[8]:2 =# Base.Experimental.@force_compile
        global_getfield()
    end === :julia2
   Evaluated: julia === julia2

This commit fixes it up.

Previously the `:inaccessiblememonly` effect bit may be wrongly refined
when analyzing va-method, e.g.:
```julia
julia> callgetfield1(xs...) = getfield(getfield(xs, 1), 1)
callgetfield1 (generic function with 1 method)

julia> Base.infer_effects(callgetfield1, (Base.RefValue{Symbol},))
(+c,+e,!n,+t,+s,+m,+i) # inaccessiblememonly is wrongly refined here
```

This leads to wrong concrete evaluation of `callgetfield1` and thus may
result in a problem like below:
```julia
julia> const GLOBAL_XS = Ref(:julia);

julia> global_getfield() = callgetfield1(GLOBAL_XS);

julia> @test let
           Base.Experimental.@force_compile
           global_getfield()
       end === :julia
Test Passed

julia> GLOBAL_XS[] = :julia2;

julia> @test let
           Base.Experimental.@force_compile
           global_getfield()
       end === :julia2 # this fails
Test Failed at REPL[8]:1
  Expression: let
        #= REPL[8]:2 =# Base.Experimental.@force_compile
        global_getfield()
    end === :julia2
   Evaluated: julia === julia2
```

This commit fixes it up.
@aviatesk aviatesk added compiler:effects effect analysis backport 1.9 Change should be backported to release-1.9 labels Mar 2, 2023
@aviatesk aviatesk merged commit 903dbd5 into master Mar 3, 2023
@aviatesk aviatesk deleted the avi/inaccessiblememonly branch March 3, 2023 00:40
aviatesk added a commit that referenced this pull request Mar 3, 2023
…y` (#48854)

Previously the `:inaccessiblememonly` effect bit may be wrongly refined
when analyzing va-method, e.g.:
```julia
julia> callgetfield1(xs...) = getfield(getfield(xs, 1), 1)
callgetfield1 (generic function with 1 method)

julia> Base.infer_effects(callgetfield1, (Base.RefValue{Symbol},))
(+c,+e,!n,+t,+s,+m,+i) # inaccessiblememonly is wrongly refined here
```

This leads to wrong concrete evaluation of `callgetfield1` and thus may
result in a problem like below:
```julia
julia> const GLOBAL_XS = Ref(:julia);

julia> global_getfield() = callgetfield1(GLOBAL_XS);

julia> @test let
           Base.Experimental.@force_compile
           global_getfield()
       end === :julia
Test Passed

julia> GLOBAL_XS[] = :julia2;

julia> @test let
           Base.Experimental.@force_compile
           global_getfield()
       end === :julia2 # this fails
Test Failed at REPL[8]:1
  Expression: let
        #= REPL[8]:2 =# Base.Experimental.@force_compile
        global_getfield()
    end === :julia2
   Evaluated: julia === julia2
```

This commit fixes it up.
@aviatesk aviatesk mentioned this pull request Mar 3, 2023
50 tasks
KristofferC pushed a commit that referenced this pull request Mar 3, 2023
…y` (#48854)

Previously the `:inaccessiblememonly` effect bit may be wrongly refined
when analyzing va-method, e.g.:
```julia
julia> callgetfield1(xs...) = getfield(getfield(xs, 1), 1)
callgetfield1 (generic function with 1 method)

julia> Base.infer_effects(callgetfield1, (Base.RefValue{Symbol},))
(+c,+e,!n,+t,+s,+m,+i) # inaccessiblememonly is wrongly refined here
```

This leads to wrong concrete evaluation of `callgetfield1` and thus may
result in a problem like below:
```julia
julia> const GLOBAL_XS = Ref(:julia);

julia> global_getfield() = callgetfield1(GLOBAL_XS);

julia> @test let
           Base.Experimental.@force_compile
           global_getfield()
       end === :julia
Test Passed

julia> GLOBAL_XS[] = :julia2;

julia> @test let
           Base.Experimental.@force_compile
           global_getfield()
       end === :julia2 # this fails
Test Failed at REPL[8]:1
  Expression: let
        #= REPL[8]:2 =# Base.Experimental.@force_compile
        global_getfield()
    end === :julia2
   Evaluated: julia === julia2
```

This commit fixes it up.
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:effects effect analysis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants