[PATCH] D68213: [LTO] Support for embedding bitcode section during LTO

[Steven Wu via Phabricator via llvm-commits]

steven_wu added inline comments.


================
Comment at: llvm/lib/LTO/LTOBackend.cpp:335
+                       .Case("bitcode", EmbedBitcodeKind::Embed_Bitcode)
+                       .Case("marker", EmbedBitcodeKind::Embed_Marker)
+                       .Default(~0U);
----------------
tejohnson wrote:
> zapster wrote:
> > tejohnson wrote:
> > > Does this value make any sense since CmdArgs is always empty below?
> > You are right. Currently, this option value is not utterly useful, but I kept it for compatibility with clangs `-fembed-bitcode`. It seems the marker section is needed (even if empty), otherwise certain tools will complain (notably the MacOS linker). Ideally, we would have something sensible to write into the section but I am not sure about reasonable values. I was not able to find any information about consumers of these commands. Pointers in that regard would be highly appreciated.
> > 
> > Anyhow, I lean toward keeping the options for compatibility with clang and for making it simple to properly implement the `marker` case. That said, I am perfectly fine with removing the option. In that case, I guess I should get rid of `EmbedBitcodeKind` altogether and always insert the bitcode and the marker, right?
> > 
> > On the other hand, we should maybe just consolidate the option (parsing) with the clang option. Some thoughts about that in my inline comment above.
> It's unclear to me whether it is better to not provide the marker, and get a clear error, or put in an empty marker which may cause these tools to do the wrong thing. @steven_wu who might be able to provide some guidance about how this is used by the MacOS linker. Note that if someone is using MacOS to do their LTO linking, they won't even trigger this code, as that toolchain uses the old legacy LTO, which doesn't come here.
> 
> Presumably your tool doesn't use the marker section?
I don't think marker is useful for this use case. I will suggest not to have the `marker` LTO option unless we have the need. It is being tested by clang and that is probably enough coverage for testing.

I don't think any users of the legacy interface express interests in using a unified bitcode section so it is fine just to leave this out of the legacy C interface.




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

https://reviews.llvm.org/D68213