[PATCH] D152973: Reland "[gold] Add preliminary FatLTO support to the Gold plugin""

Tue Jul 25 09:00:21 PDT 2023

paulkirth added a comment.

In D152973#4531264 <https://reviews.llvm.org/D152973#4531264>, @nikic wrote:

> In D152973#4529995 <https://reviews.llvm.org/D152973#4529995>, @paulkirth wrote:
>
>> In D152973#4529866 <https://reviews.llvm.org/D152973#4529866>, @nikic wrote:
>>
>>> I don't think we are going to use the FatLTO pipeline as currently implemented in Rust, due to its bad compile-time characteristics. We need a FatLTO implementation that does not require compiling the module twice.
>>
>> That's unfortunate, and is something I hope we can address moving forward. I vaguely recall your suggestion that after D148010 <https://reviews.llvm.org/D148010> we could probably do better w.r.t. optimizing the pipeline, is that still correct?
>
> It would at least make things a good bit simpler, by removing thin vs full LTO differences from the equation.

Right, I had forgotten that patch is only about smoothing out the differences between the two pipelines. I suppose that if there are not major differences, then we could potentially use the UnifiedLTO pipeline and then (I think) we'd be able to just do the  module simplification (+ anything needed for instrumentation) for generating the bitcode , and then run module optimization after to generate the object code.  At least thats the rough idea in my head ATM. There are no doubt some unfortunate edge-cases, but I'm hoping we sort those out in the not too distant future.

>> Also, out of curiosity, how does `rustc` avoid the edge cases brought up in the initial patches, like missing certain transforms for ThinLTO + SamplePGO?
>
> I'm not sure Rust even supports SamplePGO. Maybe there is an unstable feature for it.

Indeed, I see a `-Z profile-sample--use=val` flag in the help. Seems to have landed in https://github.com/rust-lang/rust/commit/a17193dbb931ea0c8b66d82f640385bce8b4929a, a couple years ago.

> But the truth is that Rust doesn't have a dedicated fat LTO pipeline at all, it just runs the usual pipeline and then both embeds the final bitcode and emits object code. This is of course bad (for the same reasons the current full LTO pipeline is bad), and replacing it with a proper fat LTO pipeline would be great.

Agreed. I'd be very happy if this simplifies things for Rust. It's an important usecase for Fuchsia, so the sooner we can get support into `rustc` the better.

In D152973#4532043 <https://reviews.llvm.org/D152973#4532043>, @peter.smith wrote:

> In D152973#4529625 <https://reviews.llvm.org/D152973#4529625>, @paulkirth wrote:
>
>> @peter.smith @nikic @maskray @tejohnson, given that Rust and others seem to use the `.llvmbc`, I see a few ways we can move forward here.
>>
>> 1. have a hard transition (e..g., this patch), and make downstream consumers adapt now.
>> 2. have a soft transition, and mark `.llvmbc` as deprecated, then remove in the following release (so about 6 months from now).
>> 3. forgo trying to separate the old uses of `.llvmbc` from new FatLTO uses and just reuse the `.llvmbc` section name.
>> 4. ... something completely different?
>>
>> I'm not really sure what the best way to go about this would be. I'd lean towards a soft transition (2), since that seems more forgiving and less hostile towards downstream consumers, but I think there are merits for any of the approaches.  There are probably other options I haven't considered either, so thoughts and suggestions would be quite welcome.
>
> I think we can adapt fairly quickly as it is an embedded toolchain and we don't support mixing LTO objects from previous versions, I think we'll be able to cope with an instant change. May be worth an RFC/FYI on Discourse as some downstream projects merge with upstream less frequently than we do.

OK, well, it seems like maybe we should just land these now, since it sounds like both downstream consumers will be able to adapt w/o much trouble. A post on Discourse is a good idea. I should be able to do that later today.

So, if there are no concerns, are we all OK w/ landing this again modulo documentation changes?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152973/new/

https://reviews.llvm.org/D152973