-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minor improvement to inference of ntuple
#54544
Conversation
In the non-`@generated` branch, where `N` may be unknown, assert its type as `Int` so that the type of the created range can be inferred.
I'm surprised this is necessary, since there's the |
I also thought we could propagate these asserts downstream, but we can't, be it for ordinary arguments or type parameters: julia> function f(m, ::Val{N}) where N
m::Int
N::Int
(m, N)
end
f (generic function with 1 method)
julia> code_typed(f, Tuple{Any, Val})
1-element Vector{Any}:
CodeInfo(
1 ─ Core.typeassert(m, Main.Int)::Int64
│ %2 = $(Expr(:static_parameter, 1))::Any
│ Core.typeassert(%2, Main.Int)::Int64
│ %4 = $(Expr(:static_parameter, 1))::Any
│ %5 = Core.tuple(m, %4)::Tuple{Any, Any}
└── return %5
) => Tuple{Any, Any} |
Hmmm that's really weird f(m, ::Val{N}) where N @ Main REPL[4]:1
Variables
#self#::Core.Const(f)
m::Any
_::Val
∘ ─ %0 = invoke f(::Any,::Val)::Tuple{Any, Any}
@ REPL[4]:2 within `unknown scope`
1 ─ %1 = Main.Int::Core.Const(Int64)
│ Core.typeassert(m, %1)::Int64
│ @ REPL[4]:3 within `unknown scope`
│ %3 = $(Expr(:static_parameter, 1))::Any
│ %4 = Main.Int::Core.Const(Int64)
│ Core.typeassert(%3, %4)::Int64
│ @ REPL[4]:4 within `unknown scope`
│ %6 = $(Expr(:static_parameter, 1))::Any
│ %7 = Core.tuple(m, %6)::Tuple{Any, Any}
└── return %7 According to Cthulhu, the type is asserted on the original SSA value, but the values for the tuple are then taken from the unasserted SSA value. That sounds like a more general bug to me 🤔 No idea what's going on with |
The usual solution is to do |
Ref #38274 for the |
Thanks for digging that up. There is no reason to believe that will be fixed soon, so I don't see any reason we shouldn't merge this: For known |
In the non-`@generated` branch, where `N` may be unknown, assert its type as `Int` so that the type of the created range can be inferred.
In the non-
@generated
branch, whereN
may be unknown, assert its type asInt
so that the type of the created range can be inferred.Unfortunately, if the function argument could not be inferred concretely, inference is still sub-optimal:
Here, the inferred type of the closure is abstract as the type of the captured variable is unknown, but its return type can still be inferred, as witnessed for the small-
N
special cases. But for the general-N
case, the return type is lost for some reason.A further improvement to inference precision would be to replace
Tuple(f(i) for i = 1:(N::Int))
with((f(i) for i = 1:(N::Int))...,)
, but for statically unknownN
, that has worse run-time performance characteristics AFAICT. I therefore stayed with the safer, minimally-invasive change here.