This is an archive of the discontinued LLVM Phabricator instance.

[NFC][ThinLTO] Let llvm::EmbedBitcodeInModule handle serialization.
ClosedPublic

Authored by mtrofin on Sep 8 2020, 5:32 PM.

Details

Summary

llvm::EmbedBitcodeInModule handles serializing the passed-in module, if
the provided MemoryBufferRef is invalid. This is already the path taken
in one of the uses of the API - clang::EmbedBitcode, when called from
BackendConsumer::HandleTranslationUnit - so might as well do the same
here and reduce (by very little) code duplication.

The only difference this patch introduces is that the serialization happens
with ShouldPreserveUseListOrder set to true.

Diff Detail

Event Timeline

mtrofin created this revision.Sep 8 2020, 5:32 PM
mtrofin requested review of this revision.Sep 8 2020, 5:32 PM

Does this rely on the following check in EmbedBitcodeInModule returning true?

if (!isBitcode((const unsigned char *)Buf.getBufferStart(),
               (const unsigned char *)Buf.getBufferEnd())) {

I.e. I assume an empty buffer will be !isBitcode? The comments there suggest this is trying to detect whether we have bitcode vs LLVM assembly as input. Might be better to add an explicit check for an empty buffer and expand the comments (along with your current change).

mtrofin updated this revision to Diff 290763.Sep 9 2020, 10:07 AM

explicit check

tejohnson accepted this revision.Sep 10 2020, 9:56 AM

LGTM with comment updates as suggested below.

llvm/include/llvm/Bitcode/BitcodeWriter.h
159

maybe make this something like "...if the provided Buf is not bitcode (e.g. is LLVM assembly)" since that seems to be what it is trying to handle

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
4839 ↗(On Diff #290763)

Update comment.

This revision is now accepted and ready to land.Sep 10 2020, 9:56 AM
mtrofin updated this revision to Diff 291017.Sep 10 2020, 10:12 AM
mtrofin marked 2 inline comments as done.

comments

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
4839 ↗(On Diff #290763)

none of that comment applies, really

This revision was landed with ongoing or failed builds.Sep 10 2020, 10:26 AM
This revision was automatically updated to reflect the committed changes.