Skip to content

Conversation

topolarity
Copy link
Member

Restores something very close to the previous inlining behavior, without reverting #54972

This is a hack to workaround #58915 (comment) for 1.12, but we should leave an issue open so that we can put in a proper fix to inlining for the next major version.

Also improves #58915 (comment), which was a dynamic call on 1.11 and a poorly-chosen invoke of the generator fallback on 1.12. This is now an inlined version of the non-fallback path for the generator:

julia> nt = (next = zero(UInt32), prev = zero(UInt32))
(next = 0x00000000, prev = 0x00000000)

julia> f(nt) = @inline Base.setindex(nt, 2, :next)
f (generic function with 1 method)

julia> @code_warntype optimize=true f(nt)
MethodInstance for f(::@NamedTuple{next::UInt32, prev::UInt32})
  from f(nt) @ Main REPL[2]:1
Arguments
  #self#::Core.Const(Main.f)
  nt::@NamedTuple{next::UInt32, prev::UInt32}
Body::@NamedTuple{next::Int64, prev::UInt32}
1%1 =   builtin Base.getfield(nt, :prev)::UInt32%2 = %new(@NamedTuple{next::Int64, prev::UInt32}, 2, %1)::@NamedTuple{next::Int64, prev::UInt32}
└──      return %2

Restores something very close to the previous inlining behavior, without
reverting the change to statically-encode `MethodError`s in the IR.

This built-in should not actually have a high cost, but it coincidentally
encourages our compiler not to inline wide calls to generators that it
will never be able to refine to the IR / type-information it would have
gotten by exploring the more narrow call to begin with.
@topolarity topolarity added the backport 1.12 Change should be backported to release-1.12 label Jul 14, 2025
@giordano giordano removed the backport 1.12 Change should be backported to release-1.12 label Jul 14, 2025
@vtjnash
Copy link
Member

vtjnash commented Jul 14, 2025

This sounded more like a bug with compute_sparams cost? We know that function is much more expensive than dynamic dispatch, so it should never inline unless the user explicitly overrides the heuristics

@topolarity
Copy link
Member Author

topolarity commented Jul 14, 2025

This sounded more like a bug with compute_sparams cost?

It probably is, but I wasn't sure if there isn't also arguably a separate bug around whether we should inline if @generated fallbacks or not in general.

In any case, this PR is just a "spiritual revert" to get back to the old behavior before we get the proper fix(es) in

@aviatesk
Copy link
Member

I think this revert is fine. Having said that now inlining won't work in cases like the one below for example, so I still think it would be better to fix the root problem if possible.
Of course, I think a future fix would be fine.

julia> func(::Int) = :int;

julia> func(::String) = :string;

julia> callfunc(x) = func(x);

julia> callcallfunc(x) = callfunc(x);

julia> code_typed(callcallfunc, (Any,))
1-element Vector{Any}:
 CodeInfo(
1%1 =   dynamic Main.callfunc(x)::Symbol
└──      return %1
) => Symbol

@KristofferC KristofferC changed the base branch from release-1.12 to backports-release-1.12 July 15, 2025 13:39
@serenity4
Copy link
Member

I made #59013 based on #58996 (comment). Does that seem like a better approach than this PR?

@topolarity
Copy link
Member Author

I'm going to merge this (just so that we have an easy-to-justify fix on 1.12) but I like the direction you're taking in #59013 @serenity4

@topolarity topolarity merged commit 3960ef4 into JuliaLang:backports-release-1.12 Jul 15, 2025
6 of 7 checks passed
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Jul 23, 2025
…functions (JuliaLang#58996)

Restores something very close to the previous inlining behavior, without
reverting JuliaLang#54972

This is a hack to workaround
JuliaLang#58915 (comment)
for 1.12, but we should leave an issue open so that we can put in a
proper fix to inlining for the next major version.

Also improves
JuliaLang#58915 (comment),
which was a dynamic `call` on 1.11 and a poorly-chosen `invoke` of the
generator fallback on 1.12. This is now an inlined version of the
non-fallback path for the generator:
```julia
julia> nt = (next = zero(UInt32), prev = zero(UInt32))
(next = 0x00000000, prev = 0x00000000)

julia> f(nt) = @inline Base.setindex(nt, 2, :next)
f (generic function with 1 method)

julia> @code_warntype optimize=true f(nt)
MethodInstance for f(::@NamedTuple{next::UInt32, prev::UInt32})
  from f(nt) @ Main REPL[2]:1
Arguments
  #self#::Core.Const(Main.f)
  nt::@NamedTuple{next::UInt32, prev::UInt32}
Body::@NamedTuple{next::Int64, prev::UInt32}
1 ─ %1 =   builtin Base.getfield(nt, :prev)::UInt32
│   %2 = %new(@NamedTuple{next::Int64, prev::UInt32}, 2, %1)::@NamedTuple{next::Int64, prev::UInt32}
└──      return %2
```
DelveCI pushed a commit to RelationalAI/julia that referenced this pull request Jul 26, 2025
…functions (JuliaLang#58996)

Restores something very close to the previous inlining behavior, without
reverting JuliaLang#54972

This is a hack to workaround
JuliaLang#58915 (comment)
for 1.12, but we should leave an issue open so that we can put in a
proper fix to inlining for the next major version.

Also improves
JuliaLang#58915 (comment),
which was a dynamic `call` on 1.11 and a poorly-chosen `invoke` of the
generator fallback on 1.12. This is now an inlined version of the
non-fallback path for the generator:
```julia
julia> nt = (next = zero(UInt32), prev = zero(UInt32))
(next = 0x00000000, prev = 0x00000000)

julia> f(nt) = @inline Base.setindex(nt, 2, :next)
f (generic function with 1 method)

julia> @code_warntype optimize=true f(nt)
MethodInstance for f(::@NamedTuple{next::UInt32, prev::UInt32})
  from f(nt) @ Main REPL[2]:1
Arguments
  #self#::Core.Const(Main.f)
  nt::@NamedTuple{next::UInt32, prev::UInt32}
Body::@NamedTuple{next::Int64, prev::UInt32}
1 ─ %1 =   builtin Base.getfield(nt, :prev)::UInt32
│   %2 = %new(@NamedTuple{next::Int64, prev::UInt32}, 2, %1)::@NamedTuple{next::Int64, prev::UInt32}
└──      return %2
```
KristofferC pushed a commit that referenced this pull request Jul 27, 2025
…functions (#58996)

Restores something very close to the previous inlining behavior, without
reverting #54972

This is a hack to workaround
#58915 (comment)
for 1.12, but we should leave an issue open so that we can put in a
proper fix to inlining for the next major version.

Also improves
#58915 (comment),
which was a dynamic `call` on 1.11 and a poorly-chosen `invoke` of the
generator fallback on 1.12. This is now an inlined version of the
non-fallback path for the generator:
```julia
julia> nt = (next = zero(UInt32), prev = zero(UInt32))
(next = 0x00000000, prev = 0x00000000)

julia> f(nt) = @inline Base.setindex(nt, 2, :next)
f (generic function with 1 method)

julia> @code_warntype optimize=true f(nt)
MethodInstance for f(::@NamedTuple{next::UInt32, prev::UInt32})
  from f(nt) @ Main REPL[2]:1
Arguments
  #self#::Core.Const(Main.f)
  nt::@NamedTuple{next::UInt32, prev::UInt32}
Body::@NamedTuple{next::Int64, prev::UInt32}
1 ─ %1 =   builtin Base.getfield(nt, :prev)::UInt32
│   %2 = %new(@NamedTuple{next::Int64, prev::UInt32}, 2, %1)::@NamedTuple{next::Int64, prev::UInt32}
└──      return %2
```
@topolarity topolarity deleted the ct/workaround-58915 branch August 28, 2025 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants