Stream: t-compiler

Topic: When to re-run ThinLTO, for PR #71267


simulacrum (Apr 20 2020 at 14:21, on Zulip):

@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?

pnkfelix (Apr 20 2020 at 14:31, on Zulip):

I suppose its possible (that things have changed since then)

pnkfelix (Apr 20 2020 at 14:31, on Zulip):

it seems like its enough to justify landing the patch in the meantime, though

pnkfelix (Apr 20 2020 at 14:34, on Zulip):

@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 ?

simulacrum (Apr 20 2020 at 14:35, on Zulip):

Yeah I meant just our folks

simulacrum (Apr 20 2020 at 14:35, on Zulip):

I agree it's enough to justify the patch

pnkfelix (Apr 20 2020 at 14:38, on Zulip):

cc @WG-llvm , any thoughts on the above reasoning?

Hanna Kruppe (Apr 20 2020 at 14:46, on Zulip):

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?

pnkfelix (Apr 20 2020 at 14:51, on Zulip):

I think so, yes

pnkfelix (Apr 20 2020 at 14:52, on Zulip):

/me realizes that some of the names in the code itself are decidedly subpar

Hanna Kruppe (Apr 20 2020 at 15:17, on Zulip):

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.

simulacrum (Apr 20 2020 at 19:43, on Zulip):

@pnkfelix are we waiting on https://github.com/rust-lang/rust/pull/71267 for beta->stable bump? I can cherry pick commits from it

pnkfelix (Apr 20 2020 at 19:44, on Zulip):

Yeah I don't know...

pnkfelix (Apr 20 2020 at 19:44, on Zulip):

I guess I'm personally comfortable personally beta-accepting it

pnkfelix (Apr 20 2020 at 19:45, on Zulip):

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...) ...

pnkfelix (Apr 20 2020 at 19:45, on Zulip):

lets go for it, I'll mark it insta-beta-accepted based on the research I did on that LLVM blog post.

simulacrum (Apr 20 2020 at 19:47, on Zulip):

Okay, will push it up then and approve

Last update: Jun 04 2020 at 17:55UTC