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

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

Some nits, but this generally looks okay as a starting point.



================
Comment at: llvm/include/llvm/Bitcode/EmbedBitcodePass.h:31
+public:
+  EmbedBitcodePass(bool IsThinLTO, bool EmitLLVMUSeLists, bool EmitLTOSummary)
+      : IsThinLTO(IsThinLTO), EmitLLVMUSeLists(EmitLLVMUSeLists),
----------------
USe -> Use here and above


================
Comment at: llvm/include/llvm/Bitcode/EmbedBitcodePass.h:33
+      : IsThinLTO(IsThinLTO), EmitLLVMUSeLists(EmitLLVMUSeLists),
+        EmitLTOSummary(EmitLTOSummary){};
+
----------------
Stray semicolon at the end.


================
Comment at: llvm/include/llvm/Passes/PassBuilder.h:240
+  /// This builds a pipeline that runs the LTO/ThinLTO pipeline, and emits a
+  /// section containing the bitcode along size the object code generated by the
+  /// PerModuleDefaultPipeline.
----------------
size -> side


================
Comment at: llvm/include/llvm/Passes/PassBuilder.h:246
+  /// require some transformations for semantic reasons, they should explicitly
+  /// build them.
+  ModulePassManager buildFatLTODefaultPipeline(OptimizationLevel Level,
----------------
You can drop this paragraph, it should support `O0` fine.


================
Comment at: llvm/include/llvm/Passes/PassBuilder.h:248
+  ModulePassManager buildFatLTODefaultPipeline(OptimizationLevel Level,
+                                               bool ThinLTOPreLink,
+                                               bool EmitUseList,
----------------
ThinLTOPreLink -> ThinLTO as this is always a pre-link pipeline?


================
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.");
+
----------------
You can pass `true` to suppress the crash, as this is user error, not a compiler error.


================
Comment at: llvm/lib/Bitcode/Writer/EmbedBitcodePass.cpp:43
+  else
+    BitcodeWriterPass(OS, EmitLLVMUSeLists, EmitLTOSummary).run(M, AM);
+
----------------
TBH I'm not sure we need the use list option here at all, as it's debugging functionality. I don't think anyone needs use lists embedded in fat object files. Especially if this only does something with one LTO type (ThinLTOBitcodeWriter hardcodes this to false).


================
Comment at: llvm/lib/Bitcode/Writer/EmbedBitcodePass.cpp:49
+
+  llvm::GlobalVariable *EM = new llvm::GlobalVariable(
+      M, ModuleConstant->getType(), true, llvm::GlobalValue::PrivateLinkage,
----------------
Drop `llvm::` prefix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146776