Align meaning for effects and IR flags (#50313)

This fixes a longstanding todo where the IR_FLAG_EFFECT_FREE flag
actually required both :effect_free and :nothrow. After this PR,
it is equivalent to :effect_free only. The mismatch in meaning here
caused #50311. `Symbol(::String)` is :effect_free, but not :nothrow.
As a result, setting IR_FLAG_EFFECT_FREE on it was not legal. Later,
irinterp did discover that it was nothrow and set IR_FLAG_NOTHROW,
but did not have sufficient information to know that it was also
:effect_free, so it could not set that flag. With this PR,
IR_FLAG_EFFECT_FREE is set early in inference, so once irinterp
discovers IR_FLAG_NOTHROW, the call becomes DCE-eligible as
desired. Fixes #50311.

Author: Keno

base/boot.jl

@@ -510,9 +510,11 @@ end)
 
 function Symbol(s::String)
     @_foldable_meta
+    @noinline
     return _Symbol(ccall(:jl_string_ptr, Ptr{UInt8}, (Any,), s), sizeof(s), s)
 end
 function Symbol(a::Array{UInt8,1})
+    @noinline
     return _Symbol(ccall(:jl_array_ptr, Ptr{UInt8}, (Any,), a), Intrinsics.arraylen(a), a)
 end
 Symbol(s::Symbol) = s

base/compiler/abstractinterpretation.jl

@@ -2280,17 +2280,33 @@ struct RTEffects
     RTEffects(@nospecialize(rt), effects::Effects) = new(rt, effects)
 end
 
+function mark_curr_effect_flags!(sv::AbsIntState, effects::Effects)
+    if isa(sv, InferenceState)
+        if is_effect_free(effects)
+            add_curr_ssaflag!(sv, IR_FLAG_EFFECT_FREE)
+        else
+            sub_curr_ssaflag!(sv, IR_FLAG_EFFECT_FREE)
+        end
+        if is_nothrow(effects)
+            add_curr_ssaflag!(sv, IR_FLAG_NOTHROW)
+        else
+            sub_curr_ssaflag!(sv, IR_FLAG_NOTHROW)
+        end
+        if is_consistent(effects)
+            add_curr_ssaflag!(sv, IR_FLAG_CONSISTENT)
+        else
+            sub_curr_ssaflag!(sv, IR_FLAG_CONSISTENT)
+        end
+    end
+end
+
 function abstract_call(interp::AbstractInterpreter, arginfo::ArgInfo, sv::InferenceState)
     si = StmtInfo(!call_result_unused(sv, sv.currpc))
     (; rt, effects, info) = abstract_call(interp, arginfo, si, sv)
     sv.stmt_info[sv.currpc] = info
     # mark this call statement as DCE-elgible
     # TODO better to do this in a single pass based on the `info` object at the end of abstractinterpret?
-    if is_removable_if_unused(effects)
-        add_curr_ssaflag!(sv, IR_FLAG_EFFECT_FREE)
-    else
-        sub_curr_ssaflag!(sv, IR_FLAG_EFFECT_FREE)
-    end
+    mark_curr_effect_flags!(sv, effects)
     return RTEffects(rt, effects)
 end
 
@@ -2429,14 +2445,7 @@ function abstract_eval_statement_expr(interp::AbstractInterpreter, e::Expr, vtyp
     elseif ehead === :foreigncall
         (; rt, effects) = abstract_eval_foreigncall(interp, e, vtypes, sv)
         t = rt
-        if isa(sv, InferenceState)
-            # mark this call statement as DCE-elgible
-            if is_removable_if_unused(effects)
-                add_curr_ssaflag!(sv, IR_FLAG_EFFECT_FREE)
-            else
-                sub_curr_ssaflag!(sv, IR_FLAG_EFFECT_FREE)
-            end
-        end
+        mark_curr_effect_flags!(sv, effects)
     elseif ehead === :cfunction
         effects = EFFECTS_UNKNOWN
         t = e.args[1]
@@ -2558,7 +2567,7 @@ end
 function abstract_eval_statement(interp::AbstractInterpreter, @nospecialize(e), vtypes::VarTable, sv::InferenceState)
     if !isa(e, Expr)
         if isa(e, PhiNode)
-            add_curr_ssaflag!(sv, IR_FLAG_EFFECT_FREE)
+            add_curr_ssaflag!(sv, IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW)
             return abstract_eval_phi(interp, e, vtypes, sv)
         end
         return abstract_eval_special_value(interp, e, vtypes, sv)

base/compiler/ssair/inlining.jl

@@ -370,7 +370,7 @@ function ir_prepare_inlining!(insert_node!::Inserter, inline_target::Union{IRCod
     if !validate_sparams(sparam_vals)
         # N.B. This works on the caller-side argexprs, (i.e. before the va fixup below)
         sp_ssa = insert_node!(
-            effect_free(NewInstruction(Expr(:call, Core._compute_sparams, def, argexprs...), SimpleVector, topline)))
+            effect_free_and_nothrow(NewInstruction(Expr(:call, Core._compute_sparams, def, argexprs...), SimpleVector, topline)))
     end
     if def.isva
         nargs_def = Int(def.nargs::Int32)
@@ -426,7 +426,7 @@ function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector
                 inline_compact.result[idx′][:type] =
                     argextype(val, isa(val, Argument) || isa(val, Expr) ? compact : inline_compact)
                 # Everything legal in value position is guaranteed to be effect free in stmt position
-                inline_compact.result[idx′][:flag] = IR_FLAG_EFFECT_FREE
+                inline_compact.result[idx′][:flag] = IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW
                 break
             end
             inline_compact[idx′] = stmt′
@@ -702,7 +702,7 @@ function batch_inline!(ir::IRCode, todo::Vector{Pair{Int,Any}}, propagate_inboun
                 for aidx in 1:length(argexprs)
                     aexpr = argexprs[aidx]
                     if isa(aexpr, Expr) || isa(aexpr, GlobalRef)
-                        ninst = effect_free(NewInstruction(aexpr, argextype(aexpr, compact), compact.result[idx][:line]))
+                        ninst = effect_free_and_nothrow(NewInstruction(aexpr, argextype(aexpr, compact), compact.result[idx][:line]))
                         argexprs[aidx] = insert_node_here!(compact, ninst)
                     end
                 end
@@ -992,9 +992,10 @@ function flags_for_effects(effects::Effects)
     if is_consistent(effects)
         flags |= IR_FLAG_CONSISTENT
     end
-    if is_removable_if_unused(effects)
-        flags |= IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW
-    elseif is_nothrow(effects)
+    if is_effect_free(effects)
+        flags |= IR_FLAG_EFFECT_FREE
+    end
+    if is_nothrow(effects)
         flags |= IR_FLAG_NOTHROW
     end
     return flags
@@ -1650,7 +1651,7 @@ function inline_const_if_inlineable!(inst::Instruction)
         inst[:inst] = quoted(rt.val)
         return true
     end
-    inst[:flag] |= IR_FLAG_EFFECT_FREE
+    inst[:flag] |= IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW
     return false
 end
 
@@ -1773,7 +1774,7 @@ function late_inline_special_case!(
             return SomeCase(quoted(type.val))
         end
         cmp_call = Expr(:call, GlobalRef(Core, :(===)), stmt.args[2], stmt.args[3])
-        cmp_call_ssa = insert_node!(ir, idx, effect_free(NewInstruction(cmp_call, Bool)))
+        cmp_call_ssa = insert_node!(ir, idx, effect_free_and_nothrow(NewInstruction(cmp_call, Bool)))
         not_call = Expr(:call, GlobalRef(Core.Intrinsics, :not_int), cmp_call_ssa)
         return SomeCase(not_call)
     elseif length(argtypes) == 3 && istopfunction(f, :(>:))
@@ -1816,13 +1817,13 @@ end
 
 function insert_spval!(insert_node!::Inserter, spvals_ssa::SSAValue, spidx::Int, do_isdefined::Bool)
     ret = insert_node!(
-        effect_free(NewInstruction(Expr(:call, Core._svec_ref, false, spvals_ssa, spidx), Any)))
+        effect_free_and_nothrow(NewInstruction(Expr(:call, Core._svec_ref, false, spvals_ssa, spidx), Any)))
     tcheck_not = nothing
     if do_isdefined
         tcheck = insert_node!(
-            effect_free(NewInstruction(Expr(:call, Core.isa, ret, Core.TypeVar), Bool)))
+            effect_free_and_nothrow(NewInstruction(Expr(:call, Core.isa, ret, Core.TypeVar), Bool)))
         tcheck_not = insert_node!(
-            effect_free(NewInstruction(Expr(:call, not_int, tcheck), Bool)))
+            effect_free_and_nothrow(NewInstruction(Expr(:call, not_int, tcheck), Bool)))
     end
     return (ret, tcheck_not)
 end
@@ -1849,7 +1850,7 @@ function ssa_substitute_op!(insert_node!::Inserter, subst_inst::Instruction,
                 (ret, tcheck_not) = insert_spval!(insert_node!, spvals_ssa::SSAValue, spidx, maybe_undef)
                 if maybe_undef
                     insert_node!(
-                        non_effect_free(NewInstruction(Expr(:throw_undef_if_not, val.name, tcheck_not), Nothing)))
+                        NewInstruction(Expr(:throw_undef_if_not, val.name, tcheck_not), Nothing))
                 end
                 return ret
             end

base/compiler/ssair/ir.jl

@@ -316,8 +316,7 @@ function NewInstruction(inst::Instruction;
     return NewInstruction(stmt, type, info, line, flag)
 end
 @specialize
-effect_free(newinst::NewInstruction) = NewInstruction(newinst; flag=add_flag(newinst, IR_FLAG_EFFECT_FREE))
-non_effect_free(newinst::NewInstruction) = NewInstruction(newinst; flag=sub_flag(newinst, IR_FLAG_EFFECT_FREE))
+effect_free_and_nothrow(newinst::NewInstruction) = NewInstruction(newinst; flag=add_flag(newinst, IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW))
 with_flags(newinst::NewInstruction, flags::UInt8) = NewInstruction(newinst; flag=add_flag(newinst, flags))
 without_flags(newinst::NewInstruction, flags::UInt8) = NewInstruction(newinst; flag=sub_flag(newinst, flags))
 function add_flag(newinst::NewInstruction, newflag::UInt8)
@@ -1677,7 +1676,7 @@ function maybe_erase_unused!(callback::Function, compact::IncrementalCompact, id
     stmt = inst[:inst]
     stmt === nothing && return false
     inst[:type] === Bottom && return false
-    effect_free = (inst[:flag] & IR_FLAG_EFFECT_FREE) ≠ 0
+    effect_free = (inst[:flag] & (IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW)) == IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW
     effect_free || return false
     foreachssa(stmt) do val::SSAValue
         if compact.used_ssas[val.id] == 1

base/compiler/ssair/irinterp.jl

@@ -162,7 +162,7 @@ function reprocess_instruction!(interp::AbstractInterpreter, idx::Int, bb::Union
     if rt !== nothing
         if isa(rt, Const)
             ir.stmts[idx][:type] = rt
-            if is_inlineable_constant(rt.val) && !isa(inst, PhiNode) && (ir.stmts[idx][:flag] & IR_FLAG_EFFECT_FREE) != 0
+            if is_inlineable_constant(rt.val) && !isa(inst, PhiNode) && (ir.stmts[idx][:flag] & (IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW)) == IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW
                 ir.stmts[idx][:inst] = quoted(rt.val)
             end
             return true

base/compiler/ssair/passes.jl

@@ -472,7 +472,7 @@ function lift_arg!(
         end
     end
     if isa(lifted, GlobalRef) || isa(lifted, Expr)
-        lifted = insert_node!(compact, leaf, effect_free(NewInstruction(lifted, argextype(lifted, compact))))
+        lifted = insert_node!(compact, leaf, effect_free_and_nothrow(NewInstruction(lifted, argextype(lifted, compact))))
         compact[leaf] = nothing
         stmt.args[argidx] = lifted
         compact[leaf] = stmt
@@ -718,7 +718,7 @@ function perform_lifting!(compact::IncrementalCompact,
         end
         if isa(old_node, PhiNode)
             new_node = PhiNode()
-            ssa = insert_node!(compact, old_ssa, effect_free(NewInstruction(new_node, result_t)))
+            ssa = insert_node!(compact, old_ssa, effect_free_and_nothrow(NewInstruction(new_node, result_t)))
             lifted_philikes[i] = LiftedPhilike(ssa, new_node, true)
         else
             @assert is_known_call(old_node, Core.ifelse, compact)
@@ -1110,8 +1110,8 @@ function sroa_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing)
                 def_val = perform_lifting!(compact,
                     visited_philikes, field, def_lifting_cache, Bool, lifted_leaves_def, val, lazydomtree).val
             end
-            insert_node!(compact, SSAValue(idx), non_effect_free(NewInstruction(
-                Expr(:throw_undef_if_not, Symbol("##getfield##"), def_val), Nothing)))
+            insert_node!(compact, SSAValue(idx), NewInstruction(
+                Expr(:throw_undef_if_not, Symbol("##getfield##"), def_val), Nothing))
 
         else
             # val must be defined

test/compiler/effects.jl

@@ -993,3 +993,8 @@ end
 hf50198(s) = hasfield(typeof((;x=1, y=2)), s)
 f50198() = (hf50198(Ref(:x)[]); nothing)
 @test fully_eliminated(f50198)
+
+# Effects properly applied to flags by irinterp (#50311)
+f50311(x, s) = Symbol(s)
+g50311(x) = Val{f50311((1.0, x), "foo")}()
+@test fully_eliminated(g50311, Tuple{Float64})