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

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added a comment.

Thanks for the feedback @tejohnson. I'm about to take off for the weekend, but I'll address these on Monday.



================
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.");
+
----------------
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.


================
Comment at: llvm/lib/Bitcode/Writer/EmbedBitcodePass.cpp:49
+  if (IsThinLTO)
+    ThinLTOBitcodeWriterPass(OS, nullptr).run(*NewModule, AM);
+  else
----------------
tejohnson wrote:
> Document constant parameter (nullptr)
ah, good catch. Thanks.


================
Comment at: llvm/test/Bitcode/embed.ll:15
+
+;; Make sue the metadata correlates to the .llvm.lto section
+; CHECK: !llvm.embedded.objects = !{!1}
----------------
tejohnson wrote:
> s/sue/sure
ty


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146776