Optimizer: Update SROA def-uses after DCE

[serenity4]

Fixes #57141.

Given the function

julia> function _f()
           ref = Ref{Any}()
           ref[] = 3
           @assert isdefined(ref, :x)
           inner = Returns(ref)
           x = inner()
           (x, ref[])
       end
_f (generic function with 1 method)

julia> f() = first(_f())
f (generic function with 1 method)

Here is before:

julia> @code_typed f()
CodeInfo(
1 ─ %1 = %new(Base.RefValue{Any})::Base.RefValue{Any}
└──      goto #3
2 ─      unreachable
3 ─      return %1
) => Base.RefValue{Any}

Here is after this PR:

julia> @code_typed f()
CodeInfo(
1 ─ %1 = %new(Base.RefValue{Any})::Base.RefValue{Any}
│          builtin Base.setfield!(%1, :x, 3)::Int64
│   %3 =   builtin Main.isdefined(%1, :x)::Bool
└──      goto #3 if not %3
2 ─      goto #4
3 ─ %6 =    invoke Base.AssertionError("isdefined(ref, :x)"::String)::AssertionError
│          builtin Base.throw(%6)::Union{}
└──      unreachable
4 ─      return %1
) => Base.RefValue{Any}

The elimination of setfield! was due to a use still being recorded for ref[] in the def-use data while DCE eliminated this getindex call (by virtue of not using the second tuple element in the result).

[serenity4]

I believe the test failure to be unrelated, I could not reproduce locally

[aviatesk]

We should probably add x::SSAUse annotation here.

[serenity4]

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

[aviatesk]

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?

[vtjnash]

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.