Skip to content

Conversation

topolarity
Copy link
Member

jl_emit_codeinst expects inference results to be available, so jl_ci_cache_lookup needs to provide a CI that has them.

I noticed this because it was causing missing code when, e.g., compiling NetworkOptions.__init__ using --trim (#55047). Unfortunately jl_emit_codeinst failures are silently ignored (they just leave the LLVM module empty), so it was easy to miss that the looked-up CIs sometimes fail to compile.

…native`

`jl_ci_cache_lookup` is used to obtain a CI that will be handed to
`jl_emit_codeinst` which expects inference results to be available.

This problem was causing missing code when, e.g., compiling
`NetworkOptions.__init__` using `--trim` (JuliaLang#55047).
@topolarity topolarity requested review from vtjnash and Keno August 19, 2024 16:12
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But note the subsequent branch also fails to provide any such guarantee either (since v1.12) so this doesn't fix the issue

@topolarity
Copy link
Member Author

the subsequent branch also fails to provide any such guarantee

Fair enough - but that's a deeper problem and this is a significant improvement for now so I'll merge this.

It would also be great to tighten up our handling of empty modules here. I don't know why we end up with an empty module sometimes (other than the caching issues introduced in v1.12)

@topolarity topolarity merged commit 378f192 into JuliaLang:master Aug 28, 2024
7 checks passed
@topolarity topolarity deleted the ct/fix-ci-cache-lookup branch August 28, 2024 16:48
KristofferC pushed a commit that referenced this pull request Sep 12, 2024
…native` (#55528)

`jl_emit_codeinst` expects inference results to be available, so
`jl_ci_cache_lookup` needs to provide a CI that has them.

I noticed this because it was causing missing code when, e.g., compiling
`NetworkOptions.__init__` using `--trim` (#55047). Unfortunately
`jl_emit_codeinst` failures are silently ignored (they just leave the
LLVM module empty), so it was easy to miss that the looked-up CIs
sometimes fail to compile.
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.

2 participants