inlining: Use concrete-eval effects if available #47305

Keno · 2022-10-24T11:15:32Z

It is possible for concrete-eval to refine effects, but be unable to inline (because the constant is too large, c.f. #47283). However, in that case, we would still like to use the extra effect information to make sure that the optimizer can delete the statement if it turns out to be unused. There are two cases: the first is where the call is not inlineable at all. This one is simple, because we just apply the effects on the :invoke as we usually would. The second is trickier: If we do end up inlining the call, we need to apply the overriden effects to every inlined statement, because we lose the identity of the function as a whole. This is a bit nasty and I don't really like it, but I'm not sure what a better alternative would be. We could always refuse to inline calls with large-constant results (since we currently pessimize what is being inlined anyway), but I'm not sure that would be better. This is a simple solution and works for the case I have in practice, but we may want to revisit it in the future.

oscardssmith · 2022-10-24T15:29:56Z

@Keno this needs a rebase.

vtjnash · 2022-10-24T19:15:01Z

This is a bit nasty and I don't really like it

I feel like this is actually quite sane. The part I most dislike is the Union{Nothing}. Can we use EFFECTS_UNKNOWN′ instead, to instead specify that we have no effects that need to be narrowed?

aviatesk · 2022-10-25T02:56:04Z

base/compiler/ssair/inlining.jl

@@ -370,7 +370,8 @@ end

 function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector{Any},
                         linetable::Vector{LineInfoNode}, item::InliningTodo,
-                         boundscheck::Symbol, todo_bbs::Vector{Tuple{Int, Int}})
+                         boundscheck::Symbol, todo_bbs::Vector{Tuple{Int, Int}},
+                         extra_flags::UInt8)


Suggested change

extra_flags::UInt8)

extra_flags::UInt8 = flags_for_effects(item.effects))

aviatesk · 2022-10-25T03:01:05Z

base/compiler/ssair/inlining.jl

@@ -967,11 +982,11 @@ function analyze_method!(match::MethodMatch, argtypes::Vector{Any},
    mi = specialize_method(match; preexisting=true) # Union{Nothing, MethodInstance}
    if mi === nothing
        et = InliningEdgeTracker(state.et, invokesig)
-        return compileable_specialization(match, Effects(), et, info;
+        return compileable_specialization(match, override_effects === nothing ? Effects() : override_effects, et, info;


I think we can do something like:

Suggested change

return compileable_specialization(match, override_effects === nothing ? Effects() : override_effects, et, info;

return compileable_specialization(match, override_effects === nothing ? info_effects(nothing, match, state) : override_effects, et, info;

and get more precision.

aviatesk · 2022-10-25T03:04:54Z

base/compiler/ssair/inlining.jl

@@ -445,6 +447,14 @@ function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector
                stmt′ = PhiNode(Int32[edge+bb_offset for edge in stmt′.edges], stmt′.values)
            end
            inline_compact[idx′] = stmt′
+            if extra_flags != 0 && !isa(stmt′, Union{GotoNode, GotoIfNot})
+                if (extra_flags & IR_FLAG_NOTHROW) != 0 && inline_compact[SSAValue(idx′)][:type] === Union{}


I think we may need this sort of tweak for the other effects, e.g. :consistent-cy can now be proved in a presence of (unreturned) mutable allocations but it sounds quite wrong to mark the mutable allocation as :consistent.
Having said that, this may be enough for the meanwhile since we only look for nothrow and effect_free in our optimization passes, and I'm fine with leaving these handling for the future when the potential problem comes up.

Keno · 2022-10-27T06:32:46Z

As CI points out, this doesn't actually work for effect_free. It only works for nothrow. Fortunately, that's all I need for the test and my actual application.

It is possible for concrete-eval to refine effects, but be unable to inline (because the constant is too large, c.f. #47283). However, in that case, we would still like to use the extra effect information to make sure that the optimizer can delete the statement if it turns out to be unused. There are two cases: the first is where the call is not inlineable at all. This one is simple, because we just apply the effects on the :invoke as we usually would. The second is trickier: If we do end up inlining the call, we need to apply the overriden effects to every inlined statement, because we lose the identity of the function as a whole. This is a bit nasty and I don't really like it, but I'm not sure what a better alternative would be. We could always refuse to inline calls with large-constant results (since we currently pessimize what is being inlined anyway), but I'm not sure that would be better. This is a simple solution and works for the case I have in practice, but we may want to revisit it in the future.

aviatesk · 2022-10-28T05:24:15Z

It only works for nothrow. Fortunately, that's all I need for the test and my actual application.

There might be a tricky consideration even for :nothrow, e.g. when @assume_effects :nothrow-annotated method contains try/catch block:

julia> Base.@assume_effects :nothrow @inline function fff(x)
           local v
           try
               v = sin(x)
           catch err
               v = 0.0
           end
           v
       end

This seems to never be a problem though, since our DCE pass isn't able to handle complicated control flows.

Keno · 2022-10-28T15:02:46Z

Yeah, you're right. This just doesn't work. I think for the time being, we just need to avoid trying to inline concrete-eval calls at all.

This undoes some of the choices made in #47283 and #47305. As Shuhei correctly pointed out, even with the restriction to `nothrow`, adding the extra flags on the inlined statements results in incorrect IR. Also, my bigger motivating test case turns out to be insufficiently optimized without the effect_free flags (which I removed in the final revision of #47305). I think for the time being, the best course of action here is to just stop inlining concrete-eval'ed calls entirely. The const result is available for inference, so in most cases the call will get deleted. If there's an important case we care about where this does not happen, we should take a look at that separately.

…rge (#47371) This undoes some of the choices made in #47283 and #47305. As Shuhei correctly pointed out, even with the restriction to `nothrow`, adding the extra flags on the inlined statements results in incorrect IR. Also, my bigger motivating test case turns out to be insufficiently optimized without the effect_free flags (which I removed in the final revision of #47305). I think for the time being, the best course of action here is to just stop inlining concrete-eval'ed calls entirely. The const result is available for inference, so in most cases the call will get deleted. If there's an important case we care about where this does not happen, we should take a look at that separately.

So that they can be deleted during the first `compact!`-ion. This allows us to delete an inlineable and effect-free, but unused call. This is essentially an alternative of #47305, but doesn't introduce a problem like #47374.

This keyword argument was introduced by #47305, which was then reverted by #47371. Now it's dead, so let's remove it.

…arg (#47762) This keyword argument was introduced by #47305, which was then reverted by #47371. Now it's dead, so let's remove it.

So that they can be deleted during the first `compact!`-ion. This allows us to delete an inlineable and effect-free, but unused call. This is essentially an alternative of #47305, but doesn't introduce a problem like #47374.

#47689) So that they can be deleted during the first `compact!`-ion. This allows us to delete an inlineable and effect-free, but unused call. This is essentially an alternative of #47305, but doesn't introduce a problem like #47374.

…compact!`-ion In code like below ```julia Base.@assume_effects :nothrow function erase_before_inlining(x, y) z = sin(y) if x return "julia" end return z end let y::Float64 length(erase_before_inlining(true, y)) end ``` the constant prop' can figure out the constant return type of `erase_before_inlining(true, y)` while it is profitable not to inline expand it since otherwise we left some `!:nothrow` callees there (xref: #47305). In order to workaround this problem, this commit makes `compact!`move inlineable constants into statement positions so that the such "inlineable, but safe as a whole" calls to be erased during compaction. This should give us general compile-time performance improvement too as we no longer need to expand the IR for those calls.

…compact!`-ion In code like below ```julia Base.@assume_effects :nothrow function erase_before_inlining(x, y) z = sin(y) if x return "julia" end return z end let y::Float64 length(erase_before_inlining(true, y)) end ``` the constant prop' can figure out the constant return type of `erase_before_inlining(true, y)` while it is profitable not to inline expand it since otherwise we left some `!:nothrow` callees there (xref: #47305). In order to workaround this problem, this commit makes `compact!`move inlineable constants into statement positions so that such "inlineable, but safe as a whole" calls to be erased during compaction. This should give us general compile-time performance improvement too as we no longer need to expand the IR for those calls.

…ompact!`-ion (#50767) In code like below ```julia Base.@assume_effects :nothrow function erase_before_inlining(x, y) z = sin(y) if x return "julia" end return z end let y::Float64 length(erase_before_inlining(true, y)) end ``` the constant prop' can figure out the constant return type of `erase_before_inlining(true, y)` while it is profitable not to inline expand it since otherwise we left some `!:nothrow` callees there (xref: #47305). In order to workaround this problem, this commit makes `compact!`move inlineable constants into argument positions so that the such "inlineable, but safe as a whole" calls to be erased during compaction. This should give us general compile-time performance improvement too as we no longer need to expand the IR for those calls. Requires: - #50764 - #50765 - #50768

Base automatically changed from kf/unionsplitinfo to master October 24, 2022 13:16

Keno force-pushed the kf/cinlineffects branch from 19567df to d696360 Compare October 24, 2022 15:39

aviatesk reviewed Oct 25, 2022

View reviewed changes

Keno force-pushed the kf/cinlineffects branch from d696360 to 71e77fd Compare October 27, 2022 07:18

Keno merged commit 6baa9a6 into master Oct 27, 2022

Keno deleted the kf/cinlineffects branch October 27, 2022 09:52

Keno mentioned this pull request Oct 28, 2022

inlining: Don't inline concrete-eval'ed calls whose result was too large #47371

Merged

aviatesk mentioned this pull request Nov 24, 2022

inference: mark flag for effect-free :calls during abstractinterpret #47689

Merged

aviatesk added a commit that referenced this pull request Dec 1, 2022

inlining: follow up #47371, remove unused override_effects keyword arg

762a20b

This keyword argument was introduced by #47305, which was then reverted by #47371. Now it's dead, so let's remove it.

aviatesk mentioned this pull request Dec 1, 2022

inlining: follow up #47371, remove unused override_effects keyword arg #47762

Merged

aviatesk added a commit that referenced this pull request Dec 1, 2022

inlining: follow up #47371, remove unused override_effects keyword …

a9e6b5a

…arg (#47762) This keyword argument was introduced by #47305, which was then reverted by #47371. Now it's dead, so let's remove it.

aviatesk mentioned this pull request Aug 2, 2023

optimizer: move inlineable constants into argument position during compact!-ion #50767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inlining: Use concrete-eval effects if available #47305

inlining: Use concrete-eval effects if available #47305

Keno commented Oct 24, 2022

oscardssmith commented Oct 24, 2022

vtjnash commented Oct 24, 2022

aviatesk Oct 25, 2022

aviatesk Oct 25, 2022

aviatesk Oct 25, 2022

Keno commented Oct 27, 2022

aviatesk commented Oct 28, 2022

Keno commented Oct 28, 2022

	extra_flags::UInt8)
	extra_flags::UInt8 = flags_for_effects(item.effects))

	return compileable_specialization(match, override_effects === nothing ? Effects() : override_effects, et, info;
	return compileable_specialization(match, override_effects === nothing ? info_effects(nothing, match, state) : override_effects, et, info;

inlining: Use concrete-eval effects if available #47305

inlining: Use concrete-eval effects if available #47305

Conversation

Keno commented Oct 24, 2022

oscardssmith commented Oct 24, 2022

vtjnash commented Oct 24, 2022

aviatesk Oct 25, 2022

Choose a reason for hiding this comment

aviatesk Oct 25, 2022

Choose a reason for hiding this comment

aviatesk Oct 25, 2022

Choose a reason for hiding this comment

Keno commented Oct 27, 2022

aviatesk commented Oct 28, 2022

Keno commented Oct 28, 2022