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

inference: prioritize force_constant_prop over const_prop_entry_heuristic #41882

Merged
merged 1 commit into from
Aug 24, 2021

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Aug 13, 2021

Currently our constant-prop' heuristics work in the following way:

  1. const_prop_entry_heuristic
  2. const_prop_argument_heuristic & const_prop_rettype_heuristic
  3. force_const_prop custom heuristic & !const_prop_function_heuristic
  4. MethodInstance specialization and const_prop_methodinstance_heuristic

This PR changes it so that the step 1. now works like:

  1. force_const_prop custom heuristic & const_prop_entry_heuristic

and the steps 2., 3. and 4. don't change

This change particularly allows us to more forcibly constant-propagate
for getproperty and setproperty!, and inline them more, e.g.:

# if we don't force constant-prop', `T = fieldtype(Foo, ::Symbol)` will be union-split to
# `Union{Type{Any},Type{Int}` and it will make `convert(T, nothing)` too costly
# and it leads to inlining failure
mutable struct Foo
    val
    _::Int
end

function setter(xs)
    for x in xs
        x.val = nothing # `setproperty!` can be inlined with this PR
    end
end

(motivated by @vchuravy's example at JuliaParallel/MPI.jl#492)

It might be useful because now we can intervene into the constant-prop'
heuristic in a more reliable way with the aggressive_constprop interface.

I did the simple benchmark below, and it looks like this change doesn't
cause the latency problem for this particular example:

~/julia master aviatesk@amdci2 6s
❯ ./usr/bin/julia -e '@time using Plots; @time plot(rand(10,3))'
  3.708500 seconds (7.28 M allocations: 506.128 MiB, 3.45% gc time, 1.13% compilation time)
  2.817794 seconds (3.45 M allocations: 195.127 MiB, 7.84% gc time, 53.76% compilation time)

~/julia avi/forceconstantprop aviatesk@amdci2 6s
❯ ./usr/bin/julia -e '@time using Plots; @time plot(rand(10,3))'
  3.622109 seconds (7.02 M allocations: 481.710 MiB, 4.19% gc time, 1.17% compilation time)
  2.863419 seconds (3.44 M allocations: 194.210 MiB, 8.02% gc time, 53.53% compilation time)

@aviatesk aviatesk requested review from Keno and vtjnash August 13, 2021 12:39
@aviatesk aviatesk force-pushed the avi/forceconstprop branch from e6eb2cd to b6478f6 Compare August 13, 2021 12:47
@aviatesk aviatesk added the compiler:inference Type inference label Aug 13, 2021
@vtjnash
Copy link
Member

vtjnash commented Aug 13, 2021

These seem a bit confusingly named. Of the "heuristic" functions, only const_prop_argument_heuristic implements heuristics for whether more analysis of the arguments are likely profitable, while the others implement checks for whether the result seems possible to improve.

@aviatesk
Copy link
Member Author

So how about these renames ?

  • const_prop_entry_heuristic -> const_prop_entry_check
  • const_prop_argument_heuristic: unchanged
  • const_prop_rettype_heuristic -> const_prop_rettype_check
  • const_prop_function_heuristic: unchanged
  • const_prop_methodinstance_heuristic: unchanged

To me const_prop_function_heuristic and const_prop_methodinstance_heuristic still seem to implement some heuristic logic rather than optimal analytic checks.

FWIW, JET seems to be the only consumer of those functions for now, and so I guess these renames won't be much painful at this moment: https://juliahub.com/ui/Search?type=code&q=const_prop&r=true

@aviatesk aviatesk force-pushed the avi/forceconstprop branch from b6478f6 to 849696a Compare August 14, 2021 12:55
aviatesk added a commit that referenced this pull request Aug 14, 2021
Built on top of #41882, this PR sorts out the constant-prop' interface
yet more, particularly generalizes `force_const_prop` to `const_prop_config`
so that it can turn on and off each heuristic.

The main motivation here is, in #41882, we want to force const-prop' on
`setproperty` even when its return type is already `Const` for the sake
of succeeding inlining, by skipping all the `const_prop_xxx_heuristic` checks.
But I still found we want to apply `const_prop_entry_heuristic` to
`getproperty`, because if we already know very accurate result for a
`getproperty` call, usually there is really no motivation for constant-prop', e.g.:
```julia
struct FZero end
Base.getproperty(::FZero, ::Symbol) = 0.0
getproperty(FZero(), :val) # const-prop' doesn't need to happen here
```

Now `force_const_prop(...) -> force::Bool` is refactored to
`const_prop_config(...) -> config::UInt8`, which can turn on and off
each heuristic based on the value of `config`.

I also included another refactoring that inlines `const_prop_rettype_heuristic`
into `const_prop_argument_heuristic`, because they really seem tightly coupled.
@vtjnash
Copy link
Member

vtjnash commented Aug 14, 2021

Ah, I missed those two also. I suppose from your example some might want to change these to add heuristics too? The main question in my mind was whether it made sense for 'force' to override them, since checks are somewhat different from heuristics.

1. `const_prop_entry_heuristic`
2. `const_prop_argument_heuristic` & `const_prop_rettype_heuristic`
3. `force_const_prop` custom heuristic & `!const_prop_function_heuristic`
4. `MethodInstance` specialization and `const_prop_methodinstance_heuristic`

This PR changes it so that the step 1. now works like:

1. `force_const_prop` custom heuristic & `const_prop_entry_heuristic`

and the steps 2., 3. and 4. don't change

This change particularly allows us to more forcibly constant-propagate
for `getproperty` and `setproperty!`, and inline them more, e.g.:
```julia
# if we don't force constant-prop', `T = fieldtype(Foo, ::Symbol)` will be union-split to
# `Union{Type{Any},Type{Int}` and it will make `convert(T, nothing)` too costly
# and it leads to inlining failure
mutable struct Foo
    val
    _::Int
end

function setter(xs)
    for x in xs
        x.val = nothing # `setproperty!` can be inlined with this PR
    end
end
```

It might be useful because now we can intervene into the constant-prop'
heuristic in a more reliable way with the `aggressive_constprop` interface.

I did the simple benchmark below, and it looks like this change doesn't
cause the latency problem for this particular example:
```zsh
~/julia master aviatesk@amdci2 6s
❯ ./usr/bin/julia -e '@time using Plots; @time plot(rand(10,3))'
  3.708500 seconds (7.28 M allocations: 506.128 MiB, 3.45% gc time, 1.13% compilation time)
  2.817794 seconds (3.45 M allocations: 195.127 MiB, 7.84% gc time, 53.76% compilation time)

~/julia avi/forceconstantprop aviatesk@amdci2 6s
❯ ./usr/bin/julia -e '@time using Plots; @time plot(rand(10,3))'
  3.622109 seconds (7.02 M allocations: 481.710 MiB, 4.19% gc time, 1.17% compilation time)
  2.863419 seconds (3.44 M allocations: 194.210 MiB, 8.02% gc time, 53.53% compilation time)
```
@aviatesk aviatesk force-pushed the avi/forceconstprop branch from 849696a to 3985cec Compare August 23, 2021 15:16
@aviatesk
Copy link
Member Author

yeah, currently force overwrites const_prop_entry_heuristic/const_prop_function_heuristic/!const_prop_methodinstance_heuristic, so I think it's more consistent if it overwrites the remaining two.

#41889 will generalize this interface and it will also generalize this force overwrite logic as well.

aviatesk added a commit that referenced this pull request Aug 23, 2021
Built on top of #41882, this PR sorts out the constant-prop' interface
yet more, particularly generalizes `force_const_prop` to `const_prop_config`
so that it can turn on and off each heuristic.

The main motivation here is, in #41882, we want to force const-prop' on
`setproperty` even when its return type is already `Const` for the sake
of succeeding inlining, by skipping all the `const_prop_xxx_heuristic` checks.
But I still found we want to apply `const_prop_entry_heuristic` to
`getproperty`, because if we already know very accurate result for a
`getproperty` call, usually there is really no motivation for constant-prop', e.g.:
```julia
struct FZero end
Base.getproperty(::FZero, ::Symbol) = 0.0
getproperty(FZero(), :val) # const-prop' doesn't need to happen here
```

Now `force_const_prop(...) -> force::Bool` is refactored to
`const_prop_config(...) -> config::UInt8`, which can turn on and off
each heuristic based on the value of `config`.

I also included another refactoring that inlines `const_prop_rettype_heuristic`
into `const_prop_argument_heuristic`, because they really seem tightly coupled.
aviatesk added a commit to JuliaDebug/Cthulhu.jl that referenced this pull request Aug 23, 2021
…e heuristics

<JuliaLang/julia#41882> will enforce const-prop'
on `getproperty` calls, and so we will get const callsites for both
`getproperty(::F32, ::Const(:val))` and `getproperty(::FZero, ::Const(:val))`.
```julia
struct F32
    val::Float32
    _v::Int
end
struct FZero end
Base.getproperty(::FZero, ::Symbol) = 0.0

find_callsites_by_ftt((Union{F32,FZero},); optimize = false) do f
    f.val
end
```

<JuliaLang/julia#41889> will further change the
heuristic so that we don't constant-fold `getproperty` as far as the
result is already `Const`, but I don't think this PR is still worth to
have because I think we will try to make sure we constant prop'
`getindex(::Const(::Tuple), ::Const)`.
aviatesk added a commit to JuliaDebug/Cthulhu.jl that referenced this pull request Aug 23, 2021
…e heuristics

<JuliaLang/julia#41882> will enforce const-prop'
on `getproperty` calls, and so we will get const callsites for both
`getproperty(::F32, ::Const(:val))` and `getproperty(::FZero, ::Const(:val))`.
```julia
struct F32
    val::Float32
    _v::Int
end
struct FZero end
Base.getproperty(::FZero, ::Symbol) = 0.0

find_callsites_by_ftt((Union{F32,FZero},); optimize = false) do f
    f.val
end
```

<JuliaLang/julia#41889> will further change the
heuristic so that we don't constant-fold `getproperty` as far as the
result is already `Const`, but I don't think this PR is still worth to
have because I think we will try to make sure we constant prop'
`getindex(::Const(::Tuple), ::Const)`.
@aviatesk aviatesk merged commit cdd2f30 into master Aug 24, 2021
@aviatesk aviatesk deleted the avi/forceconstprop branch August 24, 2021 04:21
aviatesk added a commit to JuliaDebug/Cthulhu.jl that referenced this pull request Aug 24, 2021
…e heuristics (#220)

<JuliaLang/julia#41882> will enforce const-prop'
on `getproperty` calls, and so we will get const callsites for both
`getproperty(::F32, ::Const(:val))` and `getproperty(::FZero, ::Const(:val))`.
```julia
struct F32
    val::Float32
    _v::Int
end
struct FZero end
Base.getproperty(::FZero, ::Symbol) = 0.0

find_callsites_by_ftt((Union{F32,FZero},); optimize = false) do f
    f.val
end
```

<JuliaLang/julia#41889> will further change the
heuristic so that we don't constant-fold `getproperty` as far as the
result is already `Const`, but I don't think this PR is still worth to
have because I think we will try to make sure we constant prop'
`getindex(::Const(::Tuple), ::Const)`.
aviatesk added a commit that referenced this pull request Oct 26, 2021
…ver `const_prop_entry_heuristic` (#41882)

Currently our constant-prop' heuristics work in the following way:
1. `const_prop_entry_heuristic`
2. `const_prop_argument_heuristic` & `const_prop_rettype_heuristic`
3. `force_const_prop` custom heuristic & `!const_prop_function_heuristic`
4. `MethodInstance` specialization and `const_prop_methodinstance_heuristic`

This PR changes it so that the step 1. now works like:

1. `force_const_prop` custom heuristic & `const_prop_entry_heuristic`

and the steps 2., 3. and 4. don't change

This change particularly allows us to more forcibly constant-propagate
for `getproperty` and `setproperty!`, and inline them more, e.g.:
```julia
mutable struct Foo
    val
    _::Int
end

function setter(xs)
    for x in xs
        x.val = nothing # `setproperty!` can be inlined with this PR
    end
end
```

It might be useful because now we can intervene into the constant-prop'
heuristic in a more reliable way with the `aggressive_constprop` interface.

I did the simple benchmark below, and it looks like this change doesn't
cause the latency problem for this particular example:
```zsh
~/julia master aviatesk@amdci2 6s
❯ ./usr/bin/julia -e '@time using Plots; @time plot(rand(10,3))'
  3.708500 seconds (7.28 M allocations: 506.128 MiB, 3.45% gc time, 1.13% compilation time)
  2.817794 seconds (3.45 M allocations: 195.127 MiB, 7.84% gc time, 53.76% compilation time)

~/julia avi/forceconstantprop aviatesk@amdci2 6s
❯ ./usr/bin/julia -e '@time using Plots; @time plot(rand(10,3))'
  3.622109 seconds (7.02 M allocations: 481.710 MiB, 4.19% gc time, 1.17% compilation time)
  2.863419 seconds (3.44 M allocations: 194.210 MiB, 8.02% gc time, 53.53% compilation time)
```
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
…uristic` (JuliaLang#41882)

Currently our constant-prop' heuristics work in the following way:
1. `const_prop_entry_heuristic`
2. `const_prop_argument_heuristic` & `const_prop_rettype_heuristic`
3. `force_const_prop` custom heuristic & `!const_prop_function_heuristic`
4. `MethodInstance` specialization and `const_prop_methodinstance_heuristic`

This PR changes it so that the step 1. now works like:

1. `force_const_prop` custom heuristic & `const_prop_entry_heuristic`

and the steps 2., 3. and 4. don't change

This change particularly allows us to more forcibly constant-propagate
for `getproperty` and `setproperty!`, and inline them more, e.g.:
```julia
# if we don't force constant-prop', `T = fieldtype(Foo, ::Symbol)` will be union-split to
# `Union{Type{Any},Type{Int}` and it will make `convert(T, nothing)` too costly
# and it leads to inlining failure
mutable struct Foo
    val
    _::Int
end

function setter(xs)
    for x in xs
        x.val = nothing # `setproperty!` can be inlined with this PR
    end
end
```

It might be useful because now we can intervene into the constant-prop'
heuristic in a more reliable way with the `aggressive_constprop` interface.

I did the simple benchmark below, and it looks like this change doesn't
cause the latency problem for this particular example:
```zsh
~/julia master aviatesk@amdci2 6s
❯ ./usr/bin/julia -e '@time using Plots; @time plot(rand(10,3))'
  3.708500 seconds (7.28 M allocations: 506.128 MiB, 3.45% gc time, 1.13% compilation time)
  2.817794 seconds (3.45 M allocations: 195.127 MiB, 7.84% gc time, 53.76% compilation time)

~/julia avi/forceconstantprop aviatesk@amdci2 6s
❯ ./usr/bin/julia -e '@time using Plots; @time plot(rand(10,3))'
  3.622109 seconds (7.02 M allocations: 481.710 MiB, 4.19% gc time, 1.17% compilation time)
  2.863419 seconds (3.44 M allocations: 194.210 MiB, 8.02% gc time, 53.53% compilation time)
```
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
…uristic` (JuliaLang#41882)

Currently our constant-prop' heuristics work in the following way:
1. `const_prop_entry_heuristic`
2. `const_prop_argument_heuristic` & `const_prop_rettype_heuristic`
3. `force_const_prop` custom heuristic & `!const_prop_function_heuristic`
4. `MethodInstance` specialization and `const_prop_methodinstance_heuristic`

This PR changes it so that the step 1. now works like:

1. `force_const_prop` custom heuristic & `const_prop_entry_heuristic`

and the steps 2., 3. and 4. don't change

This change particularly allows us to more forcibly constant-propagate
for `getproperty` and `setproperty!`, and inline them more, e.g.:
```julia
# if we don't force constant-prop', `T = fieldtype(Foo, ::Symbol)` will be union-split to
# `Union{Type{Any},Type{Int}` and it will make `convert(T, nothing)` too costly
# and it leads to inlining failure
mutable struct Foo
    val
    _::Int
end

function setter(xs)
    for x in xs
        x.val = nothing # `setproperty!` can be inlined with this PR
    end
end
```

It might be useful because now we can intervene into the constant-prop'
heuristic in a more reliable way with the `aggressive_constprop` interface.

I did the simple benchmark below, and it looks like this change doesn't
cause the latency problem for this particular example:
```zsh
~/julia master aviatesk@amdci2 6s
❯ ./usr/bin/julia -e '@time using Plots; @time plot(rand(10,3))'
  3.708500 seconds (7.28 M allocations: 506.128 MiB, 3.45% gc time, 1.13% compilation time)
  2.817794 seconds (3.45 M allocations: 195.127 MiB, 7.84% gc time, 53.76% compilation time)

~/julia avi/forceconstantprop aviatesk@amdci2 6s
❯ ./usr/bin/julia -e '@time using Plots; @time plot(rand(10,3))'
  3.622109 seconds (7.02 M allocations: 481.710 MiB, 4.19% gc time, 1.17% compilation time)
  2.863419 seconds (3.44 M allocations: 194.210 MiB, 8.02% gc time, 53.53% compilation time)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants