post-opt analysis bug: wrongly refine :consistent of Base._getindex(::Base.OneTo{Int}, ::Int)

[aviatesk]

From https://discourse.julialang.org/t/trying-to-understand-inferred-effects/110854.
Base._getindex(::Base.OneTo{Int}, ::Int) is expected to be !:consistent since it does have @boundscheck, yet post-optimization analysis erroneously refines it as :consistent. Specifically, the post-optimization analysis of Base._getindex(::Base.OneTo{Int}, ::Int) does the refinement with analyzing all_retpaths_consistent=true (

julia/base/compiler/optimize.jl

Line 651 in 71f68b4

consistent = sv.all_retpaths_consistent ? ALWAYS_TRUE : effects.consistent,

). However, as evident in the Cthulhu view, this method contains a GotoIfNot whose condition is Expr(:boundscheck), that analysis is wrong.

_getindex(v::UnitRange{T}, i::Integer) where T<:Union{Bool, Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8} @ Base range.jl:931
    ∘ ── %0 = invoke _getindex(::UnitRange{Int64},::Int64)::Int64 (+c,+e,!n,+t,+s,+m,+u)                                                                 
932 1 ── %1  = Base.getfield(v, :start)::Int64                                                                                                       │╻   getproperty
    │    %2  = Base.sub_int(i, 1)::Int64                                                                                                             │╻   -
    │    %3  = Base.add_int(%1, %2)::Int64                                                                                                           │╻   +
933 │    %4  = $(Expr(:boundscheck))::Bool                                                                                                           │   
    └───       goto #10 if not %4                                                                                                                    │   
    2 ── %6  = Base.slt_int(0, i)::Bool                                                                                                              │╻╷  _in_unit_range
    └───       goto #6 if not %6                                                                                                                     ││  
    3 ── %8  = Base.getfield(v, :stop)::Int64                                                                                                        ││╻   getproperty
    │    %9  = Base.sle_int(%3, %8)::Bool                                                                                                            ││╻   <=
    └───       goto #5 if not %9                                                                                                                     ││  
    4 ── %11 = Base.getfield(v, :start)::Int64                                                                                                       ││╻   getproperty
    │    %12 = Base.sle_int(%11, %3)::Bool                                                                                                           │││╻   <=
    └───       goto #7                                                                                                                               │   
    5 ──       goto #7                                                                                                                               │   
    6 ──       goto #7                                                                                                                               │   
    7 ┄─ %16 = φ (#4 => %12, #5 => false, #6 => false)::Bool                                                                                         │   
    └───       goto #9 if not %16                                                                                                                    │   
    8 ──       goto #10                                                                                                                              │   
    9 ──       invoke Base.throw_boundserror(v::UnitRange{Int64}, i::Int64)::Union{}                                                                 │   
    └───       unreachable                                                                                                                           │   
    10 ┄       return %3                                                                                                                             │   
Select a call to descend into or ↩ to ascend. [q]uit. [b]ookmark.
Toggles: [w]arn, [h]ide type-stable statements, [t]ype annotations, [s]yntax highlight for Source/LLVM/Native, [j]ump to source always, [o]ptimize, [d]ebuginfo, [r]emarks, [e]ffects, e[x]ception types, [i]nlining costs.
Show: [S]ource code, [A]ST, [T]yped code, [L]LVM IR, [N]ative code
Actions: [E]dit source code, [R]evise and redisplay
Advanced: dump [P]arams cache.
 • %19 = invoke throw_boundserror(::UnitRange{Int64},::Int64)::Union{} (+c,+e,!n,+t,+s,+m,+u)
   ↩

The underlying issue may reside in the lazyagdomtree logic, necessitating a deeper understanding of the code's purpose.

julia/base/compiler/optimize.jl

Lines 840 to 851 in 71f68b4

	(; cfg, domtree) = get!(sv.lazyagdomtree)
	for succ in iterated_dominance_frontier(cfg, BlockLiveness(sv.ir.cfg.blocks[bb].succs, nothing), domtree)
	if succ == length(cfg.blocks)
	# Phi node in the virtual exit -> We have a conditional
	# return. TODO: Check if all the retvals are egal.
	sv.all_retpaths_consistent = false
	else
	visit_bb_phis!(sv.ir, succ) do phiidx::Int
	push!(sv.inconsistent, phiidx)
	end
	end
	end

/cc @Keno

[Keno]

Why is the analysis wrong? The return value does not depend on the :boundscheck condition.

[aviatesk]

Doesn't :consistent currently includes the termination consistency? If some inconsistency can lead to throwing error, I believe it's not all_retpaths_consistent.

So I guess this is a bug of conditional_successors_may_throw not hitting properly actually?

[Keno]

Ah yes, fair point. I did write that in the spec, but then I forgot about it ;).

[Keno]

(Though I have been thinking that maybe the two notions of value consistency and termination consistency ought to be separated, but we should fix this at least.

[aviatesk]

I'm in favor of the separation.