Skip to content
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

undo breaking change of removing parent field from CodeInfo #53393

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Feb 19, 2024

This drops the unnecessary breaking change from #53219 by re-adding the optional parent field to CodeInfo. A few months ago, I had actually already put together a branch that also fixed Keno's complaints about how it was not set reliably, so I copied that code here also, so that it should get set more reliably whenever a CodeInfo is associated with a MethodInstance (either because it called retrieve_code_info to get IR from the Method or uncompress_ir to get it from the inference cache). This does not entirely fix Cthulhu's test errors, since I don't see any particular reason to re-introduce the other fields (min_world, max_world, inferred, edges, method_for_inference_limit_heuristics) that got removed or are now set incorrectly, and most errors appear to be instead related to the Expr(:boundscheck, true/false) change. However, they could be trivially re-added back as placeholders, if someone encounters a broken package that still needs them.

@vtjnash vtjnash added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Feb 19, 2024
@Keno
Copy link
Member

Keno commented Feb 21, 2024

I still think this is semantic nonsense. Cthulhu needs to be fixed anyway, since it's just making bad assumptions about the relationships of CodeInfo and MethodInstance. So 👎 from me on this change, but if you really want it, I guess I won't stand in the way.

The loss provided no value, as it is easy to provide this info, and has
downstream users as well as being documented for use.
pre_13 would fail to read the max_world field, resulting in the stream
getting desynchronized and corrupted. Add some type-asserts to help
detect that error earlier.
@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing don't squash Don't squash merge labels Feb 21, 2024
@vtjnash
Copy link
Member Author

vtjnash commented Feb 21, 2024

I agree that this value should already be known to the caller, so it should commonly be redundant here. However, our public codegen interface (code_typed) operates at the level of returning CodeInfo to define source code operations and, without the parent field, those lack some of the info required to correctly interpret them (because they lack access to the debug metadata and static_parameters).

There are also some cases in codegen where the parent value would not be quite the same as the contextual info in the caller, because of a particular rare case of codegen despecialization that means the invoke pointer is not generated from the inferred field but rather copied from elsewhere. We don't copy the inferred field info in that case, since it is unnecessary and because we cannot represent that change of parent in the compression format, but I have wondered if we should make it possible to represent that, so that reflection tools can return a more accurate idea of what gf.c/codegen.cpp will choose to do.

@vtjnash vtjnash merged commit 962d833 into master Feb 21, 2024
7 of 9 checks passed
@vtjnash vtjnash deleted the jn/codeinfo-parent branch February 21, 2024 20:18
@inkydragon inkydragon removed the merge me PR is reviewed. Merge when all tests are passing label Feb 23, 2024
tecosaur pushed a commit to tecosaur/julia that referenced this pull request Mar 4, 2024
…ang#53393)

This drops the unnecessary breaking change from
JuliaLang#53219 by re-adding the optional
`parent` field to CodeInfo. A few months ago, I had actually already put
together a branch that also fixed Keno's complaints about how it was not
set reliably, so I copied that code here also, so that it should get set
more reliably whenever a CodeInfo is associated with a MethodInstance
(either because it called `retrieve_code_info` to get IR from the Method
or `uncompress_ir` to get it from the inference cache). This does not
entirely fix Cthulhu's test errors, since I don't see any particular
reason to re-introduce the other fields (min_world, max_world, inferred,
edges, method_for_inference_limit_heuristics) that got removed or are
now set incorrectly, and most errors appear to be instead related to the
`Expr(:boundscheck, true/false)` change. However, they could be
trivially re-added back as placeholders, if someone encounters a broken
package that still needs them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) don't squash Don't squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants