@pnkfelix it might make sense to ping some of the llvm folks as well -- some of those may have changed I guess?
We'd probably also want clarification on whether that's necessary for correctness or just performance (since for incremental we mostly care about correctness). e.g. if we export too much that probably doesn't matter much in practice, for Rust code, anyway?
I suppose its possible (that things have changed since then)
it seems like its enough to justify landing the patch in the meantime, though
@simulacrum what do you think is best way to informally reach out to LLVM folks about this? Or do you "just" mean the LLVM experts within the Rust community, e.g. wg-llvm ?
Yeah I meant just our folks
I agree it's enough to justify the patch
cc @WG-llvm , any thoughts on the above reasoning?
I am fuzzy on the terminology used here. Does "(d) the content of any of the modules it exports to has changed" in https://github.com/rust-lang/rust/pull/71267#issuecomment-616581678 mean: in the case where CGU 1 and CGU 2 pull in data from the summary/index generated from CGU 3 (e.g., making some small function available for inlining), the question is whether we need to re-do CGU 3 when either CGU 1 or CGU 2 changes?
I think so, yes
/me realizes that some of the names in the code itself are decidedly subpar
From the architecture of ThinLTO, I really wouldn't expect that CGU 1 or 2 can influence CGU 3 at all if CGU 3 doesn't itself read from the summary of 1 or 2. This seems counter to the parallel-except-as-mediated-by-summaries nature of ThinLTO.
@pnkfelix are we waiting on https://github.com/rust-lang/rust/pull/71267 for beta->stable bump? I can cherry pick commits from it
Yeah I don't know...
I guess I'm personally comfortable personally beta-accepting it
normally I'd prefer to put it through a round of T-compiler review, but in this particular case, given that its literally just removing code from a prior beta backport (and therefore has ... essentially zero chance of injecting a new regression beyond what was the case before that backport...) ...
lets go for it, I'll mark it insta-beta-accepted based on the research I did on that LLVM blog post.
Okay, will push it up then and approve