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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

I'm okay with the new approach -- I think it makes the feature somewhat less compelling, but it's the path of least resistance for now. However, I'd like to have it documented that we're explicitly not guaranteeing that the results are bit-for-bit identical, and this is just an artifact of the current implementation.

Two notes on the larger feature:

- I'm concerned about the lld-only limitation. Unless there is some technical limitation here, this should be supported in LLVMgold as well.
- It looks like this requires passing `-fat-lto-objects` to lld to actually get LTO -- and it looks like the clang doesn't do this itself even if `-ffat-lto-objects` is passed to the driver? Can you please check how these flags work in GCC? I'm not entirely confident on this, but I expect that if you have fat objects they will use LTO by default unless you pass `-fno-lto` to the driver. We should try to match whatever the GCC behavior is here.



================
Comment at: llvm/docs/FatLTOSupport.rst:28
+#) Run the pre-link (Thin)LTO pipeline using the cloned module.
+#) Embed the pre-link bitcode a special `.llvm.lto` section.
+#) Optimize and the unmodified copy of the module using the normal compilation pipeline.
----------------



================
Comment at: llvm/docs/FatLTOSupport.rst:29
+#) Embed the pre-link bitcode a special `.llvm.lto` section.
+#) Optimize and the unmodified copy of the module using the normal compilation pipeline.
+#) Emit the object file, including the new `.llvm.lto` section.
----------------



================
Comment at: llvm/docs/FatLTOSupport.rst:34
+pipeline, saving the module in a new ``.llvm.lto`` section, and then running
+the `ModuleSimplificationPipeline` and the `ModuleOptimizationPipeline` to
+generate optimized object code for the module.
----------------



================
Comment at: llvm/docs/FatLTOSupport.rst:38-40
+   We conservatively run independent pipelines that are identical to those used
+   outside of FatLTO, so that we can guarantee that the compiled artifacts are
+   identical to those produced by the default and (Thin)LTO pipelines.
----------------



================
Comment at: llvm/docs/FatLTOSupport.rst:43
+Internally, the ``.llvm.lto`` section is created by running the
+``EmbedBitcodePass`` directly after the (Thin)LTO pre-link pipeline. When
+compiling for (Thin)LTO, this is normally the point at which the compiler would
----------------
The pre-link pipeline is run as part of EmbedBitcode, not before it.


================
Comment at: llvm/docs/FatLTOSupport.rst:50
+At the end of this process the compiler can emit standard object files which
+contain both the machine code in ``.text`` and the bitcode in ``.llvm.lto``.
+
----------------
This seems like the important part for end users, so maybe put that in the introduction.


================
Comment at: llvm/docs/FatLTOSupport.rst:60
+may change in the future, but extending support to other linkers isn't planned
+for now.
+
----------------
This is a very annoying limitation -- is it particularly hard to implementation support for this in the LLVMgold plugin?

We won't be able to use fat objects if it's only supported by LLD.


================
Comment at: llvm/docs/ReleaseNotes.rst:85
+  both machine code and LTO compatible bitcode. See the `RFC
+  <https://discourse.llvm.org/t/rfc-ffat-lto-objects-support/63977>`_ for more
+  details.
----------------
Maybe link the docs instead / as well here?


================
Comment at: llvm/lib/Bitcode/Writer/EmbedBitcodePass.cpp:30
+  if (M.getGlobalVariable("llvm.embedded.module", true))
+    report_fatal_error("Can only embed the module once.");
+
----------------
paulkirth wrote:
> tejohnson wrote:
> > nikic wrote:
> > > You can pass `true` to suppress the crash, as this is user error, not a compiler error.
> > true seems to be the default, did you mean false? The same could probably be said for the later call to report_fatal_error (ELF format). I actually think this one seems less like a user error and more like a compiler error (if this pass gets run twice somehow).
> Oh, yeah, probably. I do see your point though. I guess maybe `user error` encompasses adding a pass multiple times? 
> 
> IMO they're all kind of hard errors. I'm happy to do this either way, but if we want to suppress this one, then you're right that it should be `false` here.
Sorry, I meant false here.

My general rule of thumb is that if you can write a test for it, then it should not crash.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146776