add helpers for backedge updates on CI equivalence swaps and copies

[vtjnash]

No description provided.

[topolarity]

This was the originally claimed reason for #62022, which turned out to be wrong

Do you mean #62021 (comment) ?

I never claimed any reason for #62022, which is a distinct MWE

FWIW I think this was hit in the wild in timholy/Revise.jl#994, if that helps generate an MWE

[topolarity]

It is somewhat awkward that this leaves your max_world potentially incoherent with your forward edges

That seems likely to mislead us, e.g. we can now serialize a CI thinking (correctly) that it is still valid (.max_world == typemax) and then discover during de-serialization that it ends up invalid (despite loading in exactly the same method / binding state)

[vtjnash]

That's fair. Maybe we should throw this method in the world_lock, so that ordering isn't so important here (and also prevents concurrent update attempts to edges).

[topolarity]

This code seems to have a number of serious bugs / confusion still:

1. src can end up coming from callee but compiled and attached to cached if !ci_has_invoke(cached) which is quite illegal. We also forget to update code_cache(workqueue.interp)[mi] if link_ci_equiv! fails validation
2. collectinvokes! seems to be confused about whether it should be walking the old or re-linked :invoke graph (JIT does not need to walk the re-linked graph, but IIUC AOT does)
3. jl_link_ci_equiv does not check that cached->min_world <= callee->min_world so this can still create invalid-to-invoke code at lower world ages
4. cached is likely to fail validation often in bounded-world environments for reasons unrelated to concurrency, since jl_get_ci_equiv does not check that cached->max_world >= callee->max_world

It also doesn't resolve the MWE in #62021 (which I'm quite sure is this bug)

FWIW having no great test strategy for bounded-world inference / IPO divergence seems like a real problem, although not one we have to fix right away.

The testability barrier here seems to mean we can only catch cache bugs like the above by inspection or by crowd-sourcing them to our users which is a problem we should try to fix with some smart design eventually I think, like the ability to mock the cache perhaps.

[vtjnash]

collectinvokes! seems to be confused about whether it should be walking the old or re-linked :invoke graph (JIT does not need to walk the re-linked graph, but IIUC AOT does)
jl_link_ci_equiv does not check that cached->min_world <= callee->min_world so this can still create invalid-to-invoke code at lower world ages
cached is likely to fail validation often in bounded-world environments for reasons unrelated to concurrency, since jl_get_ci_equiv does not check that cached->max_world >= callee->max_world

Oooh, right. We should perhaps do 2 lookups here (first one try one that can replace the invoke, but if that fails for AOT, opportunistically compile something else)? That code in jl_get_ci_equiv was removed in a8216e0, but could be restored.

src can end up coming from callee but compiled and attached to cached if !ci_has_invoke(cached) which is quite illegal. We also forget to update code_cache(workqueue.interp)[mi] if link_ci_equiv! fails validation

Why is that illegal? Do you just mean we might have set up the edges inaccurately? We intended to show the CI are equivalent, but originally from 608a06c might never have done that

[topolarity]

Oooh, right. We should perhaps do 2 lookups here (first one try one that can replace the invoke, but if that fails for AOT, opportunistically compile something else)? That code in jl_get_ci_equiv was removed in a8216e0, but could be restored.

Yeah, that might be better. I've also been wondering if it might be worth handling this "re-linking" as part of the re-inference / re-validation that we do when loading pkgimages? That might make it possible to implement this as an atomic edge update, rather than having to preserve callee as a valid CodeInstance while also sort of avoiding its usage due to not having code / being cached / etc.

Why is that illegal? Do you just mean we might have set up the edges inaccurately?

Yeah essentially - link_ci_equiv! looks like it sets up cached to be called instead of callee, but this CodeInfo swap can mean that you invoke callee's code instead of cached which seems to require the edge in the other direction

[vtjnash]

link_ci_equiv!

Okay, that's fine then. The premise under which I was going to add that was wrong anyways (this is just preparing a dissociated list of compilation jobs--there's no implied connection until linking, and JIT/AOT already has the logic to add the edge when it is actually needed)

[topolarity]

link_ci_equiv!

Okay, that's fine then. The premise under which I was going to add that was wrong anyways (this is just preparing a dissociated list of compilation jobs--there's no implied connection until linking, and JIT/AOT already has the logic to add the edge when it is actually needed)

Hmm not sure I understand

My concern is that if cached gains an invoke generated from the src of callee, then the edges on the cached CI are now out-of-sync with the code its invoke is pointing to. The "re-link" is implicit in the swap in the compilation queue, and it bypasses the explicit JIT / AOT link logic since the CI itself has been re-linked.

No longer includes that part of the change, only some of the helper methods for re-visiting this later after some other concurrent work is resolved