[PATCH] D146776: [llvm] Preliminary fat-lto-objects support

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added inline comments.


================
Comment at: llvm/lib/Bitcode/Writer/EmbedBitcodePass.cpp:53
+
+  EM->setSection(".llvm.lto");
+  EM->setAlignment(Align(1));
----------------
nikic wrote:
> Does this get assigned the correct section flags? I'd expect it to need SHF_EXCLUDE so it doesn't end up in the final binary when linked without LTO.
It seems to work fine w/ the current implementation, and the `.llvm.lto` section doesn't end up in the final binary.  We actually have several tests to that effect added later in the stack.

The caveat there is that this stack of patches uses some custom logic for the `.llvm.lto` section in LLD. This was originally something our intern worked on, which was based on similar patches in D130777. I'm now trying to get that work into a landable state, so if we can achieve something similar w/ the existing section flags, I'd prefer to use that mechanism over some bespoke logic.

It looks like we usually handle this type of thing with named metadata, and that the section flags are set in `TargetLoweringObjectFileImpl.cpp`. 

I'll take a pass at adopting that approach instead, since it seems like more idiomatic and less brittle solution.


================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:1461
+  MPM.addPass(EmbedBitcodePass(ThinLTOPreLink, EmitUseList, EmitSummary));
+  addPerModuleDefaultPipelinePasses(MPM, Level, /*LTOPreLink=*/false);
   return MPM;
----------------
nikic wrote:
> paulkirth wrote:
> > nikic wrote:
> > > Running the full default pipeline after the pre-link pipeline doesn't make a lot of sense. You should only run the module optimization pipeline here. For ThinLTO, the pre-link pipeline + module optimization will give you pretty much exactly the default pipeline. For FatLTO, it will run module optimization twice, at least until D148010 lands.
> > > 
> > > This will still have some problems though, such as running the optimization pipeline extension points twice.
> > > Running the full default pipeline after the pre-link pipeline doesn't make a lot of sense. You should only run the module optimization pipeline here. For ThinLTO, the pre-link pipeline + module optimization will give you pretty much exactly the default pipeline. 
> > 
> > This is a very helpful detail, so thanks for pointing that out.
> > 
> > >For FatLTO, it will run module optimization twice, at least until D148010 lands.
> > > 
> > 
> > Just for clarification, when you say `FatLTO` are you referring to full/normal LTO? In my experience `FatLTO` has typically referred to the feature being implemented in this patch, since it uses a fat object file format for LTO purposes, and has been common in GCC for a long time. The only place I've seen `FatLTO` used otherwise is in the Rust compiler, so I want to be sure I'm understanding you correctly.
> > 
> > > This will still have some problems though, such as running the optimization pipeline extension points twice.
> > 
> > Do you have any recommendations about how we may address that? or at least suggestions on where to focus investigation?
> > Just for clarification, when you say FatLTO are you referring to full/normal LTO? In my experience FatLTO has typically referred to the feature being implemented in this patch, since it uses a fat object file format for LTO purposes, and has been common in GCC for a long time. The only place I've seen FatLTO used otherwise is in the Rust compiler, so I want to be sure I'm understanding you correctly.
> 
> Yes, I was using fat as in non-thin here :) It's unfortunate that that the term "fat LTO" can now refer to both "non-thin LTO" and "fat-object LTO", including the peculiar combination of "fat thin LTO"...
> 
> > Do you have any recommendations about how we may address that? or at least suggestions on where to focus investigation?
> 
> The way to observe this would be to try `-ffat-lto-object` together with something like `-fsanitize=address`. You should see address sanitizer getting run twice.
> 
> As to addressing it, I'm not sure there's a super clean way to do that short of adding an extra parameter to buildThinLTOPreLinkDefaultPipeline to suppress running the relevant extension points.
> Yes, I was using fat as in non-thin here :) It's unfortunate that that the term "fat LTO" can now refer to both "non-thin LTO" and "fat-object LTO", including the peculiar combination of "fat thin LTO"...
> 

Even more concerning: it could also be `ThinFatLTO` ;) 

so we're really in it now. Good thing English is so unambiguous and never leads to miscommunication.

> > Do you have any recommendations about how we may address that? or at least suggestions on where to focus investigation?
> 
> The way to observe this would be to try `-ffat-lto-object` together with something like `-fsanitize=address`. You should see address sanitizer getting run twice.
> 
> As to addressing it, I'm not sure there's a super clean way to do that short of adding an extra parameter to buildThinLTOPreLinkDefaultPipeline to suppress running the relevant extension points.

At first glance that doesn't look too bad, so that is probably what I'll try. 

Also thanks for the pointer on ASAN that will be useful as I try and clean this up.  :)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146776