Skip to content

Waitall!: Work around const-prop failure in setproperty!#492

Closed
vchuravy wants to merge 2 commits intomasterfrom
vc/force_setfield
Closed

Waitall!: Work around const-prop failure in setproperty!#492
vchuravy wants to merge 2 commits intomasterfrom
vc/force_setfield

Conversation

@vchuravy
Copy link
Copy Markdown
Member

@aviatesk any ideas why setproperty! failed to const-prop?

On 1.7 I am seeing:

julia> using Cthulhu

julia> using MPI

julia> descend(MPI.Waitall!, Tuple{Vector{MPI.Request}})

462 └───        invoke Base.setproperty!(%84::MPI.Request, :buffer::Symbol, MPI.nothing::Nothing)::Any

   %95  = invoke setproperty!(::MPI.Request,::Symbol,::Nothing)::Any

@aviatesk
Copy link
Copy Markdown

This is because the return type is already accurate enough and so there is no need for constant-prop' in terms of type-inference: especially, it hits this line with is_improvable(Const(nothing)) => false:
https://github.com/JuliaLang/julia/blob/f19b9a20dde2688c642b7dd709b5d9446e4df2f6/base/compiler/abstractinterpretation.jl#L605

I wonder we actually don't need constant-prop' here, since inlinear will inline this setproperty! to setfield! anyway ? But I agree that Cthulhu output is misleading for this case since it can be interpreted as "there is a constant-prop' failure".

@vchuravy
Copy link
Copy Markdown
Member Author

The issue is that this is actually poisoning codegen. So the produced LLVM IR is suboptimal.

@aviatesk
Copy link
Copy Markdown

Yes, it looks like this actually prevents inlining:

julia> code_typed(MPI.Waitall!, (Vector{MPI.Request},); optimize=true) |> 
           first
CodeInfo(
...
28 ─ %83 = Base.arrayref(true, %11, %74)::Int32
│          Base.setfield!(%76, :val, %83)::Int32
└───       invoke Base.setproperty!(%76::MPI.Request, :buffer::Symbol, MPI.nothing::Nothing)::Any
...
) => Vector{MPI.Status}

This seems better to be fixed, I will look into the solutions.

For now, manual inlining as you did looks reasonable.

@aviatesk
Copy link
Copy Markdown

It turns out the inlining failure comes from the fact that Base.convert inside setproperty! costs too much w/o constant prop' when there is Any-typed field:

setproperty!(x, f::Symbol, v) in Base at Base.jl:43
│ ─ %-1  = invoke setproperty!(::MPI.Request,::Symbol,::Nothing)::
43 10 %1 = Base.fieldtype(MPI.Request, _3)::Union{Type{Any}, Type{Int32}}1 %2 = (isa)(%1, Type{Any})::Bool                                 │
   └──    0      goto #3 if not %2                                          │
   20      goto #4                                                    │
   31000 %5 = Base.convert(%1, _4)::Core.Const(nothing)                  │
   └──    0      goto #4                                                    │
   40 %7 = φ (#2 => nothing, #3 => %5)::Core.Const(nothing)           │3 %8 = Base.setfield!(_2, _3, %7)::Core.Const(nothing)            │
   └──    0      return %8

aviatesk added a commit to JuliaLang/julia that referenced this pull request Aug 13, 2021
…uristic`

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. `!const_prop_function_heuristic` with `force_const_prop`'s custom heuristic
4. `MethodInstance` specialization and `const_prop_methodinstance_heuristic`

This PR changes it so that it now works:
1. `force_const_prop` custom heuristic & `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 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
```
(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:
```zsh
❯ ./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)

❯ ./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)
```
@simonbyrne
Copy link
Copy Markdown
Member

Should this be closed then?

@aviatesk
Copy link
Copy Markdown

This inlining failure is fixed on the very latest master. I think it's still reasonable to have this change if you work on v1.7 or lower.

@vchuravy
Copy link
Copy Markdown
Member Author

I only needed this to get Enzyme to differentiate through some MPI.jl code. I don't think it matters for other use-cases.

@vchuravy vchuravy closed this Aug 25, 2021
@giordano giordano deleted the vc/force_setfield branch September 16, 2022 19:27
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.

3 participants