JuliaLang
Refactor CodeInfo/CodeInstance separation and interfaces
The CodeInfo type is one of the oldest types in the system and has grown a bit of cruft. In particular, the rettype, inferred, parent, edges, min_world, max_world fields are not used for the original purpose of representing code, but for one or more of (in decreasing order of badness):
- Smuggling extra results from inference into the compiler
- Sumggling extra arguments into OpaqueClosure constructors
- Passing extra information from generated functions to inference
The first of these points in particular causes a fair bit of mixup between caching concerns and compiler concerns and results in external abstract interpreters maintainging their own dummy CodeInfos, just to comply with the interface. Originally, I just wanted to clean up that bit, but it didn't really make sense as a standalone piece, so this PR is more comprehensive.
In particular, this PR:
-
Removes the
parent,inferredandrettypefields ofCodeInfo. They are largely vestigal and code accessing these is probably doing the wrong thing. They should instead be looking at either the CodeInstance or remembering the query that was asked of the cache in the first place. -
Makes
edges,min_worldandmax_worldused for generated functions only. All other uses were replaced by appropriate queries on the CodeInstance. In particular, inference no longer sets these. In the future we may want to consider removing these also and having generated functions return some other object, but that is a topic to revisit once the broader compiler plugins landscape is more clear. -
Makes the external type inference interface return
CodeInstancerather thanCodeInfo. This results in a lot of cleanup, because many functions had multiple code paths, some for CodeInstance and others for fallback to inference/CodeInfo. This is all cleaned up now. If you don't have a CodeInstance, you can ask inference for one. This CodeInstance may or may not be in the cache, but you can look at its types, compile it, etc. -
Moves the main inference entrypoint out of the codegen library. There is still a little bit of entangelement, but this makes codegen much more of an independent system that you give a CodeInstance and it just fills in the invoke pointer for.
With these changes, only the third use of the above mentioned fields remains.
The overall theme here is decoupling. Over time, various parties have wanted to use the julia compiler with custom IR datastructure, backend code generators, caches, etc. This doesn't quite get us all the way there, but makes inference and codegen much more independent with a clear IR-format-independent interface (CodeInstance).
This will conflict with #52233 which I am hoping to merge in the next few days. I went through this PR and I didn't see anything to bad, only my unease about uncached CodeInstances :)
Planning to conduct a thorough review around tomorrow. On a broader note, maybe relevant to Valentin's PR as well, it might be worth discussing whether this change should be incorporated into v1.11. While I agree that external uses of the CodeInfo fields is usualy wrong, considering its anticipated widespread use within the community, giving some leeway for ecosystem updates could be preferred?
We're still before the feature freeze, so I think it makes sense to merge these kinds of internals-breaking changes. Otherwise, we'll merge it right after the feature freeze and we'll need to maintain both versions through the full RC period.
jinx, I also just rebased this locally ;). Let's see if we ended up in the same place ;).
jinx, I also just rebased this locally ;). Let's see if we ended up in the same place ;).
No diff modulo whitespace, so I'll just switch to this.
There's some more cleanup required here post-rebase. Are you working on that, or should I go ahead?
I've gone through and addressed the review comments. I think the biggest remaining issue is that the caching logic is not quite ideal. In particular, under contention, it may compile the same code multiple times for different CodeInstances (however, I think the same was true before also - the code paths were just very convoluted). I'd like to punt changing that work to a follow up PR, since that'll likely involve a fair bit of lock scope refactoring.
CI all green. I'm planning to merge this tomorrow. There's 2-3 follow up items still to be done, but I think they can be done in parallel on top of this.
This broke Cthulhu.jl since it accesses parent: https://github.com/JuliaDebug/Cthulhu.jl/blob/864002c4b573918a60f218e1ef86bc9a8ee3cc0f/TypedSyntax/src/node.jl#L401C1-L402C1
This broke Cthulhu.jl since it accesses parent:
https://github.com/JuliaDebug/Cthulhu.jl/pull/547
Removing parent was a breaking change. It is why I asked for that to be put back before merging this, as it doesn't seem relevant to the rest of the change, so it should be very simple to revert that part of the change without impacting anything else.
It's an internal field like the rest of the fields removed in this PR. Cthulhu's reliance on it is a bug that we'll fix.
It is documented as being part of the API here: https://github.com/JuliaLang/julia/blob/02699bb6cd83d16b1e51e2fb127241e18df7c56b/doc/src/devdocs/ast.md?plain=1#L723-L725
If it was a problem for other reasons, we could argue to make a breaking change to remove that, but as it is, it is simple to restore that field without affecting the rest of this PR.
That's developer documentation, which does not constrain the interface. We do need to update that documentation, both for this change and several others in the past few months.
The problem with that field is that there's no good semantic point at which to set it and that any user of the field is likely doing the wrong thing. Before this PR it was just being set at some random point in inference.
It looks like this causes some fairly extreme performance issues (up to 50x longer inference times), though curiously also sometimes provides up to a 5x speed up