Mutable let block variable treated as immutable in Julia 1.10

[omlins]

MWE:

let
    global set_a, get_a
    _a::Int       = -1
    set_a(a::Int) = (_a = a; return get_a())
    get_a()       = _a
end
    
set_a(1)
get_a()
set_a(2)
get_a()

Wrong output in Julia 1.10:

julia> let
           global set_a, get_a
           _a::Int       = -1
           set_a(a::Int) = (_a = a; return get_a())
           get_a()       = _a
       end
get_a (generic function with 1 method)

julia> set_a(1)
-1

julia> get_a()
-1

julia> set_a(2)
-1

julia> get_a()
-1

Correct output in Julia 1.9.4:

julia> let
           global set_a, get_a
           _a::Int       = -1
           set_a(a::Int) = (_a = a; return get_a())
           get_a()       = _a
       end
get_a (generic function with 1 method)

julia> set_a(1)
1

julia> get_a()
1

julia> set_a(2)
2

julia> get_a()
2

[vchuravy]

@aviatesk this seems possibly effects related. It seems like we might miss-model the setfield/getfield on the box.

julia> @code_llvm set_a(2)
;  @ REPL[1]:4 within `set_a`
define i64 @julia_set_a_371(i64 signext %0) #0 {
top:
  ret i64 -1
}

julia> @code_typed set_a(2)
CodeInfo(
1 ─     nothing::Nothing
│       Core.setfield!(Core.Box(-1), :contents, a)::Int64
└──     return -1
) => Int64

julia> @code_llvm optimize=false set_a(2)
;  @ REPL[1]:4 within `set_a`
define i64 @julia_set_a_386(i64 signext %0) #0 {
top:
  %1 = call {}*** @julia.get_pgcstack()
  %2 = bitcast {}*** %1 to {}**
  %current_task = getelementptr inbounds {}*, {}** %2, i64 -14
  %3 = bitcast {}** %current_task to i64*
  %world_age = getelementptr inbounds i64, i64* %3, i64 15
  ret i64 -1
}

[vchuravy]

∘ ─ %0 = invoke set_a(::Int64)::Core.Const(-1) (+c,+e,+n,+t,+s,+m,+i)
    @ REPL[1]:4 within `set_a`
1 ─ %1  = a::Int64
│   %2  = Core.Box(-1)::Core.Const(Core.Box(-1))
│         (@_3 = %1)::Int64
│         (@_3 isa Main.Int)::Core.Const(true) (+c,+e,+n,+t,+s,+m,+i)
│         nothing::Any
└──       goto #3
2 ─       Core.Const(:(Base.convert(Main.Int, @_3)))::Union{}
└──       Core.Const(:(@_3 = Core.typeassert(%7, Main.Int)))::Union{}
3 ┄ %9  = @_3::Int64
│         Core.setfield!(%2, :contents, %9)::Int64 (+c,?e,+n,+t,+s,?m,+i)
│   %11 = Main.get_a()::Core.Const(-1) (+c,+e,+n,+t,+s,+m,+i)
└──       return %11

Why is get_a consistent?

∘ ─ %0 = invoke get_a()::Int64 (+c,+e,!n,+t,+s,+m,+i)
    @ REPL[1]:5 within `get_a`
1 ─ %1 = $(QuoteNode(Core.Box(-1)))
│        Core.isdefined(%1, :contents)::Core.Const(true) (+c,+e,+n,+t,+s,?m,+i)
│        nothing::Any
└──      goto #3
2 ─      Core.Const(Core.NewvarNode(:(_a)))::Union{}
└──      Core.Const(:(_a))::Union{}
3 ┄ %7 = Core.getfield(%1, :contents)::Any (?c,+e,+n,+t,+s,?m,+i)
│   %8 = Core.typeassert(%7, Main.Int)::Int64 (+c,+e,!n,+t,+s,+m,+i)
└──      return %8

So we decided to concrete eval it, but we shouldn't have since the getfield is ?c.

[aviatesk]

I will look into this tomorrow. It's certainly a bug of the effects system.

[vchuravy]

On main we get even further xD:

julia> @code_typed set_a(2)
CodeInfo(
1 ─     return -1
) => Int64

[gbaraldi]

I think the bigger question is why is set_a(2) effect free?

[vtjnash]

If read that right, it is saying the result is consistent as long as the return value is immutable (since then we didn't use the result of that load?), which, since we asserted the return value is an Int, it thus determines the result is consistent?

[vchuravy]

julia> @eval getbox() = $(QuoteNode(Core.Box(1))).contents
getbox (generic function with 1 method)

julia> @eval getref() = $(QuoteNode(Core.Ref(1)))[]
getref (generic function with 1 method)

julia> fbox() = getbox()
fbox (generic function with 1 method)

julia> fref() = getref()
fref (generic function with 1 method)

julia> @code_typed fref()
CodeInfo(
1 ─ %1 = Base.getfield($(QuoteNode(Base.RefValue{Int64}(1))), :x)::Int64
└──      return %1
) => Int64

julia> @code_typed fbox()
CodeInfo(
1 ─     return 1
) => Int64

So maybe it's Core.Box specifically.

[vchuravy]

%1 = < constprop > getproperty(::Base.RefValue{Int64},::Core.Const(:x))::Int64 (!c,+e,+n,+t,+s,?m,+i)
│   %2 = Base.getfield(x, f)::Int64 (!c,+e,+n,+t,+s,?m,+i)

vs

%1 = < constprop > getproperty(::Core.Const(Core.Box(1)),::Core.Const(:contents))::Any (?c,+e,+n,+t,+s,?m,+i)
│   %2 = Base.getfield(x, f)::Any (?c,+e,+n,+t,+s,?m,+i)

[gbaraldi]

Those effects for box seem fine though, the weird thing is that ?m should not refine to inaccessiblememonly because Core.Box is mutable

[vchuravy]

We can also observe the same for Ref if we avoid the getindex...

julia> @eval getref() = $(QuoteNode(Core.Ref(1))).x
getref (generic function with 1 method)

julia> @code_typed fref()
CodeInfo(
1 ─     return 1
) => Int64

[gbaraldi]

julia> const a = Core.Ref(1)
Base.RefValue{Int64}(1)

julia> @eval getref() = $(QuoteNode(a)).x
getref (generic function with 1 method)

julia> Base.infer_effects(getref,())
(+c,+e,+n,+t,+s,+m,+u)

julia> getref()
1

julia> a.x = 9
9

julia> getref()
9

julia/base/compiler/typeinfer.jl

Lines 483 to 490 in 0c46852

	if is_consistent_if_inaccessiblememonly(ipo_effects)
	if is_inaccessiblememonly(ipo_effects)
	consistent = ipo_effects.consistent & ~CONSISTENT_IF_INACCESSIBLEMEMONLY
	ipo_effects = Effects(ipo_effects; consistent)
	elseif is_inaccessiblemem_or_argmemonly(ipo_effects)
	else # `:inaccessiblememonly` is already tainted, there will be no chance to refine this
	ipo_effects = Effects(ipo_effects; consistent=ALWAYS_FALSE)
	end

I suspect that the else if branch here is incorrect. We don't refine the memory effects after this, so this function will never be consistent.

[gbaraldi]

So testing just removing those elseif calls does fix my minimized case, but does not fix the original case. I believe that happens because in that case we emit a direct getfield call that doesn't go through the function that I modified, which begs the question.
When is it legal to refine a getfield with ?m to one with m, because if the memory is mutable, it's going to continue to be mutable. The only case where I can see this being consistent is if we can prove that the memory accessed here does not escape.

[gbaraldi]

I'm not sure this is 100% fixed, the original issue in ParallelStencil still seems to be there

[gbaraldi]

New MWE

julia> foo(x) = (set_initialized(true); nothing)
foo (generic function with 1 method)

julia> let
           global is_initialized, set_initialized
           _is_initialized = false
           set_initialized(flag::Bool) = (_is_initialized = flag)
           is_initialized()            = _is_initialized
       end
is_initialized (generic function with 1 method)

julia> @show is_initialized()
is_initialized() = false
false

julia> @show set_initialized(true)
set_initialized(true) = true
true

julia> @show is_initialized()
is_initialized() = true
true

julia> @show set_initialized(false)
set_initialized(false) = false
false

julia> @show is_initialized()
is_initialized() = false
false

julia> foo(4)

julia> @show is_initialized()
is_initialized() = false
false

[vchuravy]

This looks like a bug in lowering. At least @code_lowered on is_initialized returns false, but it also does so in 1.9

NVM... I didn't define set_initalize

[vchuravy]

Yes inside foo set_initalize effects are +c and +m

The Box is not lowered to be a QuoteNode. It's just a Box.

That feels wrong since Box is not defined to be self-quoting...