From 3acf8ecd104c098225876a24e1cd4d16bfae0047 Mon Sep 17 00:00:00 2001 From: Valentin Churavy Date: Sun, 12 Dec 2021 15:48:52 -0500 Subject: [PATCH 1/4] SROA: Ensure that unused structs are preserved in ccall --- base/compiler/ssair/passes.jl | 9 ++++++++- test/compiler/irpasses.jl | 27 +++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/base/compiler/ssair/passes.jl b/base/compiler/ssair/passes.jl index 000bb1849edea..abf229e768e0e 100644 --- a/base/compiler/ssair/passes.jl +++ b/base/compiler/ssair/passes.jl @@ -928,7 +928,14 @@ function sroa_mutables!(ir::IRCode, defuses::IdDict{Int, Tuple{SPCSet, SSADefUse end end preserve_uses === nothing && continue - push!(intermediaries, newidx) + preserve_newidx = false + for use in defuse.ccall_preserve_uses + if isempty(preserve_uses[use]) + preserve_newidx = true + break + end + end + preserve_newidx || push!(intermediaries, newidx) # Insert the new preserves for (use, new_preserves) in preserve_uses ir[SSAValue(use)] = form_new_preserves(ir[SSAValue(use)]::Expr, intermediaries, new_preserves) diff --git a/test/compiler/irpasses.jl b/test/compiler/irpasses.jl index dbffa41edc7ae..fea8775ddf1d3 100644 --- a/test/compiler/irpasses.jl +++ b/test/compiler/irpasses.jl @@ -672,3 +672,30 @@ let return Core.Compiler.widenconst(ft) !== typeof(typeassert) end end + +let + # Test for https://github.com/JuliaLang/julia/issues/43402 + # Ensure that structs required not used outside of the ccall, + # still get listed in the ccall_preserves + + src = @eval Module() begin + @inline function effectful() + s1 = Ref{Csize_t}() + s2 = Ref{Csize_t}() + ccall(:some_ccall, Cvoid, + (Ref{Csize_t},Ref{Csize_t}), + s1, s2) + return s1[], s2[] + end + + code_typed() do + s1, s2 = effectful() + return s1 + end |> only |> first + end + + ccall = findfirst(x -> x isa Expr && x.head == :foreigncall && x.args[1] == :(:some_ccall), src.code) + @test ccall !== nothing + ccall = src.code[ccall] + @test length(ccall.args) == 9 +end From b09c990085a640f66cb3068cf5de7b0122908e96 Mon Sep 17 00:00:00 2001 From: Valentin Churavy Date: Sun, 12 Dec 2021 16:00:47 -0500 Subject: [PATCH 2/4] SROA: Reformulate as a postitive condition --- base/compiler/ssair/passes.jl | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/base/compiler/ssair/passes.jl b/base/compiler/ssair/passes.jl index abf229e768e0e..e3e55f6476249 100644 --- a/base/compiler/ssair/passes.jl +++ b/base/compiler/ssair/passes.jl @@ -928,14 +928,13 @@ function sroa_mutables!(ir::IRCode, defuses::IdDict{Int, Tuple{SPCSet, SSADefUse end end preserve_uses === nothing && continue - preserve_newidx = false - for use in defuse.ccall_preserve_uses - if isempty(preserve_uses[use]) - preserve_newidx = true - break - end + any_field_ccall_preserve = _any(use->!isempty(preserve_uses[use]), defuse.ccall_preserve_uses) + if any_field_ccall_preserve + # this means the ccall preserve is for a field and not the whole struct + # so we can potentially eliminate the allocation, otherwise we must preserve + # the whole allocation. + push!(intermediaries, newidx) end - preserve_newidx || push!(intermediaries, newidx) # Insert the new preserves for (use, new_preserves) in preserve_uses ir[SSAValue(use)] = form_new_preserves(ir[SSAValue(use)]::Expr, intermediaries, new_preserves) @@ -945,7 +944,7 @@ function sroa_mutables!(ir::IRCode, defuses::IdDict{Int, Tuple{SPCSet, SSADefUse end end -function form_new_preserves(origex::Expr, preserved::Vector{Int}, new_preserves::Vector{Any}) +function form_new_preserves(origex::Expr, intermediates::Vector{Int}, new_preserves::Vector{Any}) newex = Expr(:foreigncall) nccallargs = length(origex.args[3]::SimpleVector) for i in 1:(6+nccallargs-1) @@ -953,7 +952,8 @@ function form_new_preserves(origex::Expr, preserved::Vector{Int}, new_preserves: end for i in (6+nccallargs):length(origex.args) x = origex.args[i] - if isa(x, SSAValue) && x.id in preserved + # don't need to preserve intermediaries + if isa(x, SSAValue) && x.id in intermediates continue end push!(newex.args, x) From e87285ea396965766149305722ef62f3aeae345d Mon Sep 17 00:00:00 2001 From: Valentin Churavy Date: Sun, 12 Dec 2021 17:18:53 -0500 Subject: [PATCH 3/4] improve test --- test/compiler/irpasses.jl | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/test/compiler/irpasses.jl b/test/compiler/irpasses.jl index fea8775ddf1d3..05fb890f91848 100644 --- a/test/compiler/irpasses.jl +++ b/test/compiler/irpasses.jl @@ -694,8 +694,13 @@ let end |> only |> first end - ccall = findfirst(x -> x isa Expr && x.head == :foreigncall && x.args[1] == :(:some_ccall), src.code) - @test ccall !== nothing - ccall = src.code[ccall] - @test length(ccall.args) == 9 + refs = map(Core.SSAValue, findall(x->x isa Expr && x.head == :new, src.code)) + some_ccall = findfirst(x -> x isa Expr && x.head == :foreigncall && x.args[1] == :(:some_ccall), src.code) + @assert some_ccall !== nothing + stmt = src.code[some_ccall] + nccallargs = length(stmt.args[3]::Core.SimpleVector) + preserves = stmt.args[6+nccallargs:end] + @test length(refs) == 2 + @test length(preserves) == 2 + @test all(alloc -> alloc in preserves, refs) end From f08b64ac8ed08db8866e1ad9ded68582e7171459 Mon Sep 17 00:00:00 2001 From: Valentin Churavy Date: Mon, 13 Dec 2021 10:00:05 -0500 Subject: [PATCH 4/4] Calculate condition as part of for-loop Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> --- base/compiler/ssair/passes.jl | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/base/compiler/ssair/passes.jl b/base/compiler/ssair/passes.jl index e3e55f6476249..0a1badff94baf 100644 --- a/base/compiler/ssair/passes.jl +++ b/base/compiler/ssair/passes.jl @@ -852,13 +852,17 @@ function sroa_mutables!(ir::IRCode, defuses::IdDict{Int, Tuple{SPCSet, SSADefUse typ = typ::DataType # Partition defuses by field fielddefuse = SSADefUse[SSADefUse() for _ = 1:fieldcount(typ)] + all_forwarded = true for use in defuse.uses stmt = ir[SSAValue(use)] # == `getfield` call # We may have discovered above that this use is dead # after the getfield elim of immutables. In that case, # it would have been deleted. That's fine, just ignore # the use in that case. - stmt === nothing && continue + if stmt === nothing + all_forwarded = false + continue + end field = try_compute_fieldidx_stmt(ir, stmt::Expr, typ) field === nothing && @goto skip push!(fielddefuse[field].uses, use) @@ -928,9 +932,8 @@ function sroa_mutables!(ir::IRCode, defuses::IdDict{Int, Tuple{SPCSet, SSADefUse end end preserve_uses === nothing && continue - any_field_ccall_preserve = _any(use->!isempty(preserve_uses[use]), defuse.ccall_preserve_uses) - if any_field_ccall_preserve - # this means the ccall preserve is for a field and not the whole struct + if all_forwarded + # this means all ccall preserves have been replaced with forwarded loads # so we can potentially eliminate the allocation, otherwise we must preserve # the whole allocation. push!(intermediaries, newidx)