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.

Sign up for GitHub

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

Optimizer: Update SROA def-uses after DCE #57201

Merged
merged 3 commits into from
Feb 3, 2025
Merged

Optimizer: Update SROA def-uses after DCE #57201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
925afc6
Update defuses based on DCE results
Jan 30, 2025
2dcdb54
Update Compiler/test/irpasses.jl
serenity4 Feb 3, 2025
f19dad9
Shorten test
Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Compiler/src/ssair/passes.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1511,6 +1511,10 @@ function sroa_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing)
used_ssas[x.id] -= 1
end
ir = complete(compact)
# remove any use that has been optimized away by the DCE
for (intermediaries, defuse) in values(defuses)
filter!(x -> ir[SSAValue(x.idx)][:stmt] !== nothing, defuse.uses)
Copy link
Member

Choose a reason for hiding this comment

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

We should probably add x::SSAUse annotation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is necessary, defuse is already typed as SSADefUse in the defuses container and .uses is also typed to be SSAUse:

struct SSADefUse
    uses::Vector{SSAUse}
    defs::Vector{Int}
end

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that’s true in general cases, but since Compiler code needs to be inferred even before inference is fully set up during bootstrap, there is a best practice of declaring method signatures as concretely as possible.

More broadly, even in cases where inference doesn’t work perfectly, having concrete method signatures allows that the method can still be compiled, making this a generally useful technique. In this specific case, inference is very likely to succeed, but given that other parts of the code follow this convention, it might make sense to do so here as well.

That said, I’m not entirely sure if this best practice is still necessary in the current implementation of Compiler.jl. What do you think on this @vtjnash?

Copy link
Member

Choose a reason for hiding this comment

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

It is mainly about readability here. A lot of untyped arguments makes it really hard to understand what is supposed to happen in the code. If only one thing can happen, then it is usually better to assert that it happened, both so that if something different happens the compiler gets an error and so the reader can see what the author expected to happen.

end
sroa_mutables!(ir, defuses, used_ssas, lazydomtree, inlining)
return ir
else
Expand Down
12 changes: 12 additions & 0 deletions Compiler/test/irpasses.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2030,3 +2030,15 @@ let code = Any[
ir = Compiler.domsort_ssa!(ir, domtree)
Compiler.verify_ir(ir)
end

# https://github.com/JuliaLang/julia/issues/57141
# don't eliminate `setfield!` when the field is to be used
let src = code_typed1(()) do
ref = Ref{Any}()
ref[] = 0
@assert isdefined(ref, :x)
inner() = ref[] + 1
(inner(), ref[])
end
@test count(iscall((src, setfield!)), src.code) == 1
end