Move inlinability determination into cache transform #57979

Keno · 2025-04-02T04:41:22Z

Currently the inlineability determination is in a bit of an odd spot - just after the optimizers while everything is still in IRCode. It seems more sensible to move this code into the cache transformation code, which is the first place that makes an actual decision based on inlineability. If an external AbstractInterpreter does not need to covert to CodeInfo for compilation purposes this also potentially saves that extra conversion. While we're at it, clean up some naming to deconflict it with other uses.

aviatesk · 2025-04-02T11:37:37Z

Compiler/src/optimize.jl

+function finishopt!(interp::AbstractInterpreter, opt::OptimizationState,
+                   ir::IRCode, caller::InferenceResult)


Suggested change

function finishopt!(interp::AbstractInterpreter, opt::OptimizationState,

ir::IRCode, caller::InferenceResult)

function finishopt!(::AbstractInterpreter, opt::OptimizationState, ir::IRCode)

aviatesk · 2025-04-02T11:37:46Z

Compiler/src/optimize.jl

    @timeit "optimizer" ir = run_passes_ipo_safe(opt.src, opt)
    ipo_dataflow_analysis!(interp, opt, ir, caller)
-    return finish(interp, opt, ir, caller)
+    finishopt!(interp, opt, ir, caller)


Suggested change

finishopt!(interp, opt, ir, caller)

finishopt!(interp, opt, ir)

aviatesk · 2025-04-02T11:47:37Z

Compiler/src/typeinfer.jl

+        if !discard_src && codegen !== nothing && uncompressed !== nothing
+            if !(uncompressed isa CodeInfo)
+                uncompressed = ir_to_codeinf!(uncompressed)
+            end


I wasn't sure why this part needed to be changed. Perhaps we could use inferred_result here instead of uncompressed? That way, if we properly define transform_result_for_cache, it would also provide support for codegen_cache.

It is also now missing a check for isa CodeInfo, which may cause problems for other consumers

vtjnash · 2025-04-02T13:29:53Z

Compiler/src/typeinfer.jl

+    if may_compress(interp)
+        return ccall(:jl_compress_ir, String, (Any, Any), def, ci)


This transform is very expensive, why is it suddenly not conditional on being useful anymore?

That case doesn't reach here anymore.

vtjnash · 2025-04-02T13:31:48Z

Compiler/src/typeinfer.jl

+        uncompressed = result.src
        const_flag = is_result_constabi_eligible(result)
        discard_src = caller.cache_mode === CACHE_MODE_NULL || const_flag
        if !discard_src


It looks like the implementation of everything after here assumes that transform_result_for_cache returns a CodeInfo for correctness, and needs to be re-written

It looks like debuginfo may still computed incorrectly on this part of the PR

@serenity4 take a look?

Although independently it does now feel a bit weird from a dataflow perspective that the debuginfo that goes with the optimized source takes a different path.

It would make more sense to me to take di = src.debuginfo (where src::CodeInfo is extracted from result.src::Union{CodeInfo, OptimizationState}) and override it with what we can extract from inferred_result (essentially doing the same logic as for result.src).

Perhaps something like that: serenity4@318b9a3

I've rebased this PR and cherry-picked your commit.

serenity4 · 2025-04-04T10:45:28Z

Digging into this, it seems that the inlining cost is expected to be set for the source CodeInfo, at least for the case where no new CodeInfo is produced post-optimization (either because results are not cached - isdefined(result, :ci) === false - or because the CodeInfo would not be inlineable, a behavior introduced in this PR).

Essentially I think the failures were related to the fact that although

It seems more sensible to move this code into the cache transformation code, which is the first place that makes an actual decision based on inlineability

the cache transformation code path may not be taken, and in that case we still need to determine inlineability of the source CodeInfo.

I have a working version locally, just needs some tree-shaking to see what are the minimally required changes. @Keno feel free to let me know if you'd like to apply the fix yourself and address review comments, or if I should take it from here (opening a new PR I guess).

serenity4 · 2025-04-04T11:03:29Z

Was this PR motivated by a need to customize inlineability determination for AbstractInterpreters? If so, and if my understanding outlined above is correct, we might need to expose that more explicitly via the AbstractInterpreter interface instead of through transform_result_for_cache as it is not the only place where inlineability determination would happen.

Keno · 2025-04-08T14:13:27Z

The primary motivation was to nail down what amount of information needs to be in the cache if you don't use CodeInfo. It seems odd to me from a conceptual dataflow perspective that inlineability needs to be set on the original CodeInfo - it's an output from the optimizer. Where you able to determine what situation reads it from there?

serenity4 · 2025-04-08T16:09:08Z

I see, I haven't looked into what piece of code required it particularly but will investigate with that perspective.

Keno · 2025-04-08T23:35:33Z

Also, once we do figure that out, I think there is an independent investigation to be had as to why not inlining something leads to a miscompile.

serenity4 · 2025-04-09T21:08:57Z

After taking a look I see that the only situation we need to set the inlining cost is when !isdefined(result, :ci) && isa(result.src, OptimizationState) && caller.cache_mode === CACHE_MODE_LOCAL. IIUC, that's when a CodeInfo is inferred and optimized and cached locally (but not globally). In this case we don't go through transform_result_for_cache, but inference may still use optimization results. Should we also call transform_result_for_cache (or some variant like transform_result_for_local_cache) in this situation? (as it's for a different cache, calling transform_result_for_cache may result in a confusing API)

Keno · 2025-04-09T21:17:11Z

Yes, I think a transform_result_for_local_cache would be appropriate.

serenity4 · 2025-04-09T22:32:37Z

Great, here is the diff between my branch and this PR if you'd like to have a look: kf/moveinlinemodel...serenity4:julia:kf/moveinlinemodel

Keno · 2025-04-09T22:48:37Z

Excellent, seems to be working! I'll look into if I can figure out why we miscompile if we don't inline those.

Keno · 2025-04-10T21:51:31Z

I tried a bunch of things and found some other issues, but I can't figure out how to make it miscompile without just going back to the previous version. I think I'll leave it be at this point, although it is a bit mysterious.

serenity4 · 2025-04-10T22:21:27Z

Thanks for taking a look, at least the additional fixes are always good to have.

Currently the inlineability determination is in a bit of an odd spot - just after the optimizers while everything is still in IRCode. It seems more sensible to move this code into the cache transformation code, which is the first place that makes an actual decision based on inlineability. If an external AbstractInterpreter does not need to covert to CodeInfo for compilation purposes this also potentially saves that extra conversion. While we're at it, clean up some naming to deconflict it with other uses.

Jameson's were already addressed in previous commits

) * 2.17: Fix `.result` -> `.optresult` change for nightly * Bump version * Adjust to more changes from JuliaLang/julia/pull/57979 * Retrieve `interp` from `OptimizationState` --------- Co-authored-by: Cédric Belmant <[email protected]>

Restores dropping these after #57979

This reverts commit b422883.

The point of #57979 was to make inference faster, but it made it instead much slower (83524ac#commitcomment-155658124), so revert back to the fast behavior before it was "optimized" (and revert the bugfixes for the original commit).

This reverts commit a7b8c83.

Reverts #58182. The API changes were intentional and desirable. Let's figure out why nansoldier was upset and re-apply this. --------- Co-authored-by: Cédric Belmant <[email protected]>

Currently the inlineability determination is in a bit of an odd spot - just after the optimizers while everything is still in IRCode. It seems more sensible to move this code into the cache transformation code, which is the first place that makes an actual decision based on inlineability. If an external AbstractInterpreter does not need to covert to CodeInfo for compilation purposes this also potentially saves that extra conversion. While we're at it, clean up some naming to deconflict it with other uses. --------- Co-authored-by: Cédric Belmant <[email protected]>

Restores dropping these after JuliaLang#57979

JuliaLang#58182) The point of JuliaLang#57979 was to make inference faster, but it made it instead much slower (JuliaLang@83524ac#commitcomment-155658124), so revert back to the fast behavior before it was "optimized" (and revert the bugfixes for the original commit).

JuliaLang#58203) Reverts JuliaLang#58182. The API changes were intentional and desirable. Let's figure out why nansoldier was upset and re-apply this. --------- Co-authored-by: Cédric Belmant <[email protected]>

JuliaLang#58182) The point of JuliaLang#57979 was to make inference faster, but it made it instead much slower (JuliaLang@83524ac#commitcomment-155658124), so revert back to the fast behavior before it was "optimized" (and revert the bugfixes for the original commit).

JuliaLang#58203) Reverts JuliaLang#58182. The API changes were intentional and desirable. Let's figure out why nansoldier was upset and re-apply this. --------- Co-authored-by: Cédric Belmant <[email protected]>

aviatesk reviewed Apr 2, 2025

View reviewed changes

vtjnash reviewed Apr 2, 2025

View reviewed changes

Keno force-pushed the kf/moveinlinemodel branch from 867c6dc to f3f6a55 Compare April 10, 2025 18:49

Keno and others added 7 commits April 11, 2025 01:45

Ensure that source CodeInfos have their inlining cost correctly set

4204d27

Refactor IR -> CodeInfo transforms and caching logic

5f3ae2d

Don't set the CodeInfo's source inlining cost unless uncached

779a5df

Introduce transform_result_for_local_cache

642564d

Address Shuhei's comments

33050a1

Jameson's were already addressed in previous commits

Improve DebugInfo extraction

e26a8a7

Keno force-pushed the kf/moveinlinemodel branch from f3f6a55 to e26a8a7 Compare April 11, 2025 01:46

Keno merged commit b422883 into master Apr 11, 2025
4 of 7 checks passed

Keno deleted the kf/moveinlinemodel branch April 11, 2025 06:22

serenity4 mentioned this pull request Apr 11, 2025

Precompilation error on nightly JuliaDebug/Cthulhu.jl#633

Closed

serenity4 added a commit to serenity4/Cthulhu.jl that referenced this pull request Apr 11, 2025

Adjust to more changes from JuliaLang/julia/pull/57979

c2379d2

serenity4 mentioned this pull request Apr 11, 2025

Adjust to .result -> .optresult change for OptimizationState JuliaDebug/Cthulhu.jl#634

Merged

Keno added a commit that referenced this pull request Apr 11, 2025

Drop sources for constabi results in local cache

385e2da

Restores dropping these after #57979

Keno mentioned this pull request Apr 11, 2025

Drop sources for constabi results in local cache #58082

Merged

Keno added a commit that referenced this pull request Apr 12, 2025

Drop sources for constabi results in local cache (#58082)

bd193e4

Restores dropping these after #57979

vtjnash added a commit that referenced this pull request Apr 21, 2025

Revert "Move inlinability determination into cache transform (#57979)"

278e84c

This reverts commit b422883.

vtjnash mentioned this pull request Apr 21, 2025

Revert #57979 (and following #58083 #58082) #58182

Merged

Keno added a commit that referenced this pull request Apr 23, 2025

Revert "Revert #57979 (and following #58083 #58082) (#58182)"

5bc8634

This reverts commit a7b8c83.

serenity4 pushed a commit to serenity4/julia that referenced this pull request May 1, 2025

Drop sources for constabi results in local cache (JuliaLang#58082)

c992286

Restores dropping these after JuliaLang#57979

		function finishopt!(interp::AbstractInterpreter, opt::OptimizationState,
		ir::IRCode, caller::InferenceResult)

	function finishopt!(interp::AbstractInterpreter, opt::OptimizationState,
	ir::IRCode, caller::InferenceResult)
	function finishopt!(::AbstractInterpreter, opt::OptimizationState, ir::IRCode)

	finishopt!(interp, opt, ir, caller)
	finishopt!(interp, opt, ir)

		if may_compress(interp)
		return ccall(:jl_compress_ir, String, (Any, Any), def, ci)

Uh oh!

Move inlinability determination into cache transform #57979

Move inlinability determination into cache transform #57979

Uh oh!

Conversation

Keno commented Apr 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vtjnash Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serenity4 commented Apr 4, 2025

Uh oh!

serenity4 commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keno commented Apr 8, 2025

Uh oh!

serenity4 commented Apr 8, 2025

Uh oh!

Keno commented Apr 8, 2025

Uh oh!

serenity4 commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keno commented Apr 9, 2025

Uh oh!

serenity4 commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keno commented Apr 9, 2025

Uh oh!

Keno commented Apr 10, 2025

Uh oh!

serenity4 commented Apr 10, 2025

Uh oh!

Uh oh!

Uh oh!

vtjnash Apr 2, 2025 •

edited

Loading

serenity4 commented Apr 4, 2025 •

edited

Loading

serenity4 commented Apr 9, 2025 •

edited

Loading

serenity4 commented Apr 9, 2025 •

edited

Loading