Decouple CrateNum of CrateInfo from the compilation session
#67516
Conversation
|
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @bjorn3 |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Not a member of |
| @@ -117,6 +116,19 @@ bitflags::bitflags! { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | |||
| pub struct CrateNum(pub u32); | |||
There was a problem hiding this comment.
This name can be confused with def_id::CrateNum.
There was a problem hiding this comment.
Good point. I chose to keep the name CrateNum mainly because I feel LinkingCrateNum is misleading. LinkingCrateNum gives an impression that this "crate num" is solely for the purpose of linking, but in fact it is just for CrateInfo to index multiple crates. In addition, this new rustc_codegen_ssa::CrateNum serves the same purpose as def_id::CrateNum does under different scope; so I decided to keep its name CrateNum. While reviewing this PR, we can think about it more and find it a better name.
|
@0dvictor Could you please add changes as extra commits instead of force-pushing them while this PR is still being reviewed. Quoting
|
Apologize for my mistake and the inconvenience. I'll definitely add any further changes as extra commits. |
|
☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Rebased to resolve merge conflicts. |
|
☔ The latest upstream changes (presumably #67886) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Rebased to resolve merge conflicts. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Rebased again to fix the error introduced by the previous rebase. |
src/librustc_codegen_ssa/lib.rs
Outdated
| fn into(self) -> def_id::CrateNum { | ||
| def_id::CrateNum::from_u32(self.0) | ||
| } | ||
| } |
There was a problem hiding this comment.
Yes, it is used in src/librustc_codegen_ssa/back/write.rs:995.
each_linked_rlib_for_lto.push((cnum.into(), path.to_path_buf()));
There was a problem hiding this comment.
I think you should use this CrateNum for each_linked_rlib_for_lto and exported_symbols too.
There was a problem hiding this comment.
Good point, updated.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #65241) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Rebased to resolve merge conflicts. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Rebased again to fix the error introduced by the previous rebase. |
There was a problem hiding this comment.
It is unclear to me what benefits this gives us. CrateNum in rustc_hir is already fairly decoupled -- I would like to see some documentation as to the difference between this CrateNum and the one in rustc_hir. We seem to be just converting into the newly introduced CrateNum in this PR, which seems odd, as then I would expect it to be the same... but if it is, then why is there a different type?
|
cc @Centril I'm going to assign myself as reviewer here though depending on the outcome of my questions may reassign to someone else (or suggest e.g. compiler team feedback via a design meeting). |
Mark-Simulacrum
added
S-waiting-on-author
This PR is to resolve this comment of PR #67195. I think @bjorn3's concern is PR #67195 is the first step to allow splitting linker invocation from the compilation process. Therefore, one invocation of @bjorn3: could you double check if I understand your original concern correctly? |
Indeed, there should not be an instance of |
`CrateNum` used inside a `CrateInfo` is self-contained and hence can be decoupled from the compilation session. By doing so, a compilation session can consume `CrateInfo` generated from a different compilation session. Ultimately, it allows splitting linker invocation into a separate compilation session.
|
Reading the discussion on that PR (#67195 (comment)) is somewhat helpful, in concert with your comment. However, I still remain skeptical that this is the right approach.
This seems incomplete -- what are you doing with the CrateNums in that crate_info data structure? I would personally expect that if we're serializing to something like |
It serves as unique identifiers for the crate's dependencies. Such unique identifiers do not have to be
Totally agree, but we still need some identifier for each dependent. Such identifier is effectively equivalent to Entry indices to In addition, when decoding the |
|
Instead of having a separate CrateNum (i.e. this PR) can we do the mapping inside the serialization step for CrateInfo? AFAICT, we can deserialize and serialize CrateNum (into just u32, if necesssary) already -- just as well as this new CrateNum -- that feels sufficient to me. |
|
Definitely, and in fact it is exactly what the current version of #67195 is doing. I’m open to either approach. |
|
Great. Let's close this then and go with that approach, I think it's better -- at least for now. |
CrateNumused inside aCrateInfois self-contained and hence can be decoupled from the compilation session. By doing so, a compilation session can consumeCrateInfogenerated from a different compilation session. Ultimately, it allows splitting linker invocation into a separate compilation session.