extend comments for reachability set computation #122769
Conversation
rustbot
added
S-waiting-on-review
RalfJung
changed the title
This comment has been minimized.
This comment has been minimized.
| //! This set is *not* transitively closed, i.e., in general the set only contains definitions that | ||
| //! can be reached *directly* via an exported name, not private functions that can only be reached | ||
| //! transitively. |
There was a problem hiding this comment.
I find this discussion of transitive closure quite confusing, since the implied relation is neither defined, nor is it what I would expect it to be.
If I were to introduce a notion of immediate reachability it would have outgoing edges only for inlinable items, and then the job of the pass is to compute transitive closure of the initial set of items. Unlike what is being discussed here.
There was a problem hiding this comment.
Why would "can the item be inlined" be relevant for reachability? That's an artifact of monomorphization and our codegen strategy, but here we are pre-monomorphization. So there's some obvious notion of "can this item be called from the outside", and there's an obvious graph of items mentioning other items, all defined on generic items. (In that graph, a trait method call has an edge to all methods that may be called there.) When I hear "reachable" I would intuitively expect that set to be closed under the edges of that graph. The point of this comment is to make it clear that that's not the intent of this pass.
IOW, under the usual meaning of the word "reachable": If A is reachable, and from A I can reach B, then clearly B is reachable. But here that property doesn't hold, which is counter-intuitive and needs to be called out.
I am not entirely following what you are saying here. Do you have a concrete suggestion?
There was a problem hiding this comment.
Why would "can the item be inlined" be relevant for reachability? That's an artifact of monomorphization and our codegen strategy, but here we are pre-monomorphization.
This pass identifies items that can be code generated in other crates and items that can be referenced at link time from other crates. The notion "can the item be inlined" is directly relevant here, since if an reachable item might be inlined, all items it mentions are reachable as well. In the opposite case, the items it mentions are irrelevant.
There is an obvious graph used here, under which this pass computes transitive closure of the initial set, so this is what I would describe.
There was a problem hiding this comment.
Hm... I'm a bit worried you might suffer from the "curse of knowledge" and so have very different expectations from people that didn't think hard about how inlining and monomorphization interact. But I'll try to adjust the docs to use your framing, and then maybe add a paragraph on how that compares to other relations one might expect this to compute.
There was a problem hiding this comment.
If we want the code to be accessible, we should really not use the term "inlined" so loosely. I'm happy to see that the new comments do not use it. We're referring to a specific behavior here, which is known in various parts of the compiler as InstantiationMode::LocalCopy (I'd call this LocalCopy codegen in English) or generates_cgu_internal_copy which is basically English already. I would much prefer we use that terminology instead of "inlined".
I tried to name the query/feature that I added cross_crate_inlinable because that is the observable effect that it is designed to produce. If that term is muddying things, we should just rename it.
There was a problem hiding this comment.
I've certainly seen confusing comments in the compiler where the English text spoke about "inlinable" and then the code called generates_cgu_internal_copy and it was not clear that they were talking about the same thing... so yeah we should stick to one term and use it throughout.
| //! However, there's a catch: if an item is generic or cross-crate inlinable, then it will have its | ||
| //! code generated by some downstream crate. Now if that item calls private monomorphic | ||
| //! non-cross-crate-inlinable items, then those can be reached by the code generated by the | ||
| //! downstream create! Therefore, when a reachable thing is cross-crate inlinable or generic, it | ||
| //! makes all other functions that it references reachable as well. |
There was a problem hiding this comment.
This is a great addition. It took me a while to understand this paragraph and its implications.
|
I think the latest wording is reasonably accessible, and that you understand quite well the perspective on these concepts I've been wanting 👍 |
|
Okay I think I resolved all comments. |
|
@bors r+ rollup=always |
bors
added
S-waiting-on-bors
also extend the const fn reachability test
|
@bors r=tmiasko |
extend comments for reachability set computation I hope this is right. :) Please review carefully. r? `@tmiasko` Cc `@oli-obk` `@saethlin`
…kingjubilee Rollup of 10 pull requests Successful merges: - rust-lang#122707 (Fix a typo in the alloc::string::String docs) - rust-lang#122769 (extend comments for reachability set computation) - rust-lang#122892 (fix(bootstrap/dist): use versioned dirs when vendoring) - rust-lang#122896 (Update stdarch submodule) - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.) - rust-lang#122950 (Add regression tests for rust-lang#101903) - rust-lang#122958 (Port backtrace dylib-dep test to a ui test) - rust-lang#123039 (Update books) - rust-lang#123044 (`Instance` is `Copy`) - rust-lang#123051 (did I mention that tests are super cool? ) r? `@ghost` `@rustbot` modify labels: rollup
…kingjubilee Rollup of 10 pull requests Successful merges: - rust-lang#122707 (Fix a typo in the alloc::string::String docs) - rust-lang#122769 (extend comments for reachability set computation) - rust-lang#122892 (fix(bootstrap/dist): use versioned dirs when vendoring) - rust-lang#122896 (Update stdarch submodule) - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.) - rust-lang#122950 (Add regression tests for rust-lang#101903) - rust-lang#123039 (Update books) - rust-lang#123042 (Import the 2021 prelude in the core crate) - rust-lang#123044 (`Instance` is `Copy`) - rust-lang#123051 (did I mention that tests are super cool? ) r? `@ghost` `@rustbot` modify labels: rollup
…kingjubilee Rollup of 10 pull requests Successful merges: - rust-lang#122707 (Fix a typo in the alloc::string::String docs) - rust-lang#122769 (extend comments for reachability set computation) - rust-lang#122892 (fix(bootstrap/dist): use versioned dirs when vendoring) - rust-lang#122896 (Update stdarch submodule) - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.) - rust-lang#122950 (Add regression tests for rust-lang#101903) - rust-lang#123039 (Update books) - rust-lang#123042 (Import the 2021 prelude in the core crate) - rust-lang#123044 (`Instance` is `Copy`) - rust-lang#123051 (did I mention that tests are super cool? ) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#122769 - RalfJung:reachable, r=tmiasko extend comments for reachability set computation I hope this is right. :) Please review carefully. r? ``@tmiasko`` Cc ``@oli-obk`` ``@saethlin``
reachable computation: extend explanation of what this does, and why Follow-up to rust-lang#122769. I had the time to think about this some more, in particular in the context of rust-lang#119214, so I felt it was worth extending these comments some more. I also gave up on the context of "externally reachable" as it is not called that way anywhere else in the compiler. Cc `@tmiasko` `@saethlin`
Rollup merge of rust-lang#124904 - RalfJung:reachable, r=tmiasko reachable computation: extend explanation of what this does, and why Follow-up to rust-lang#122769. I had the time to think about this some more, in particular in the context of rust-lang#119214, so I felt it was worth extending these comments some more. I also gave up on the context of "externally reachable" as it is not called that way anywhere else in the compiler. Cc `@tmiasko` `@saethlin`
reachable computation: extend explanation of what this does, and why Follow-up to rust-lang/rust#122769. I had the time to think about this some more, in particular in the context of rust-lang/rust#119214, so I felt it was worth extending these comments some more. I also gave up on the context of "externally reachable" as it is not called that way anywhere else in the compiler. Cc `@tmiasko` `@saethlin`
I hope this is right. :) Please review carefully.
r? @tmiasko
Cc @oli-obk @saethlin