From c81df691f27f7afbd9a8e77d559ea7a0471899f9 Mon Sep 17 00:00:00 2001 From: Teresa Johnson Date: Sat, 19 Sep 2020 10:09:28 -0700 Subject: [PATCH] [ThinLTO] Avoid temporaries when loading global decl attachment metadata When performing ThinLTO importing, the metadata loader attempts to lazy load, by building an index. However, module level global decl attachment metadata was being parsed early while building the index, since the associated (module level) global values aren't materialized on demand. This results in the creation of forward reference temporary metadatas, which are expensive. Normally, these module level global values don't have much attached metadata. However, in the case of -fwhole-program-vtables (e.g. for whole program devirtualization), the vtables may have many attached type metadatas. This was resulting in very slow performance when performing ThinLTO importing with the default lazy loading. This patch restructures the handling of these global decl attachment records, delaying their parsing until after the lazy loading index has been built. Then the parser can use the interface that loads from the index, which resolves forward references immediately instead of creating expensive temporaries. For one ThinLTO backend that imports from modules containing huge numbers of vtables and associated types, I measured the following compile times for the metadata materialization during function importing, rounded to nearest second: No -fwhole-program-vtables: Lazy loading on (head): 1s Lazy loading off (head): 3s Lazy loading on (patch): 1s With -fwhole-program-vtables: Lazy loading on (head): 440s Lazy loading off (head): 4s Lazy loading on (patch): 2s Differential Revision: https://reviews.llvm.org/D87970 --- lib/Bitcode/Reader/MetadataLoader.cpp | 126 ++++++++++++++++++++++---- test/ThinLTO/X86/devirt2.ll | 6 +- 2 files changed, 110 insertions(+), 22 deletions(-) diff --git a/lib/Bitcode/Reader/MetadataLoader.cpp b/lib/Bitcode/Reader/MetadataLoader.cpp index 874bb84170d..01ebad9594e 100644 --- a/lib/Bitcode/Reader/MetadataLoader.cpp +++ b/lib/Bitcode/Reader/MetadataLoader.cpp @@ -438,6 +438,20 @@ class MetadataLoader::MetadataLoaderImpl { /// Index that keeps track of where to find a metadata record in the stream. std::vector GlobalMetadataBitPosIndex; + /// Cursor position of the start of the global decl attachments, to enable + /// loading using the index built for lazy loading, instead of forward + /// references. + uint64_t GlobalDeclAttachmentPos = 0; + +#ifndef NDEBUG + /// Sanity check that we end up parsing all of the global decl attachments. + unsigned NumGlobalDeclAttachSkipped = 0; + unsigned NumGlobalDeclAttachParsed = 0; +#endif + + /// Load the global decl attachments, using the index built for lazy loading. + Expected loadGlobalDeclAttachments(); + /// Populate the index above to enable lazily loading of metadata, and load /// the named metadata as well as the transitively referenced global /// Metadata. @@ -681,8 +695,10 @@ Expected MetadataLoader::MetadataLoaderImpl::lazyLoadModuleMetadataBlock() { IndexCursor = Stream; SmallVector Record; + GlobalDeclAttachmentPos = 0; // Get the abbrevs, and preload record positions to make them lazy-loadable. while (true) { + uint64_t SavedPos = IndexCursor.GetCurrentBitNo(); Expected MaybeEntry = IndexCursor.advanceSkippingSubblocks( BitstreamCursor::AF_DontPopBlockAtEnd); if (!MaybeEntry) @@ -817,25 +833,11 @@ MetadataLoader::MetadataLoaderImpl::lazyLoadModuleMetadataBlock() { break; } case bitc::METADATA_GLOBAL_DECL_ATTACHMENT: { - // FIXME: we need to do this early because we don't materialize global - // value explicitly. - if (Error Err = IndexCursor.JumpToBit(CurrentPos)) - return std::move(Err); - Record.clear(); - if (Expected MaybeRecord = - IndexCursor.readRecord(Entry.ID, Record)) - ; - else - return MaybeRecord.takeError(); - if (Record.size() % 2 == 0) - return error("Invalid record"); - unsigned ValueID = Record[0]; - if (ValueID >= ValueList.size()) - return error("Invalid record"); - if (auto *GO = dyn_cast(ValueList[ValueID])) - if (Error Err = parseGlobalObjectAttachment( - *GO, ArrayRef(Record).slice(1))) - return std::move(Err); + if (!GlobalDeclAttachmentPos) + GlobalDeclAttachmentPos = SavedPos; +#ifndef NDEBUG + NumGlobalDeclAttachSkipped++; +#endif break; } case bitc::METADATA_KIND: @@ -885,6 +887,81 @@ MetadataLoader::MetadataLoaderImpl::lazyLoadModuleMetadataBlock() { } } +// Load the global decl attachments after building the lazy loading index. +// We don't load them "lazily" - all global decl attachments must be +// parsed since they aren't materialized on demand. However, by delaying +// their parsing until after the index is created, we can use the index +// instead of creating temporaries. +Expected MetadataLoader::MetadataLoaderImpl::loadGlobalDeclAttachments() { + // Nothing to do if we didn't find any of these metadata records. + if (!GlobalDeclAttachmentPos) + return true; + IndexCursor = Stream; + SmallVector Record; + // Jump to the position before the first global decl attachment, so we can + // scan for the first BitstreamEntry record. + if (Error Err = IndexCursor.JumpToBit(GlobalDeclAttachmentPos)) + return std::move(Err); + while (true) { + Expected MaybeEntry = IndexCursor.advanceSkippingSubblocks( + BitstreamCursor::AF_DontPopBlockAtEnd); + if (!MaybeEntry) + return MaybeEntry.takeError(); + BitstreamEntry Entry = MaybeEntry.get(); + + switch (Entry.Kind) { + case BitstreamEntry::SubBlock: // Handled for us already. + case BitstreamEntry::Error: + return error("Malformed block"); + case BitstreamEntry::EndBlock: + // Sanity check that we parsed them all. + assert(NumGlobalDeclAttachSkipped == NumGlobalDeclAttachParsed); + return true; + case BitstreamEntry::Record: + break; + } + uint64_t CurrentPos = IndexCursor.GetCurrentBitNo(); + Expected MaybeCode = IndexCursor.skipRecord(Entry.ID); + if (!MaybeCode) + return MaybeCode.takeError(); + if (MaybeCode.get() != bitc::METADATA_GLOBAL_DECL_ATTACHMENT) { + // Anything other than a global decl attachment signals the end of + // these records. sanity check that we parsed them all. + assert(NumGlobalDeclAttachSkipped == NumGlobalDeclAttachParsed); + return true; + } +#ifndef NDEBUG + NumGlobalDeclAttachParsed++; +#endif + // FIXME: we need to do this early because we don't materialize global + // value explicitly. + if (Error Err = IndexCursor.JumpToBit(CurrentPos)) + return std::move(Err); + Record.clear(); + if (Expected MaybeRecord = + IndexCursor.readRecord(Entry.ID, Record)) + ; + else + return MaybeRecord.takeError(); + if (Record.size() % 2 == 0) + return error("Invalid record"); + unsigned ValueID = Record[0]; + if (ValueID >= ValueList.size()) + return error("Invalid record"); + if (auto *GO = dyn_cast(ValueList[ValueID])) { + // Need to save and restore the current position since + // parseGlobalObjectAttachment will resolve all forward references which + // would require parsing from locations stored in the index. + CurrentPos = IndexCursor.GetCurrentBitNo(); + if (Error Err = parseGlobalObjectAttachment( + *GO, ArrayRef(Record).slice(1))) + return std::move(Err); + if (Error Err = IndexCursor.JumpToBit(CurrentPos)) + return std::move(Err); + } + } +} + /// Parse a METADATA_BLOCK. If ModuleLevel is true then we are parsing /// module level metadata. Error MetadataLoader::MetadataLoaderImpl::parseMetadata(bool ModuleLevel) { @@ -914,6 +991,14 @@ Error MetadataLoader::MetadataLoaderImpl::parseMetadata(bool ModuleLevel) { MetadataList.resize(MDStringRef.size() + GlobalMetadataBitPosIndex.size()); + // Now that we have built the index, load the global decl attachments + // that were deferred during that process. This avoids creating + // temporaries. + SuccessOrErr = loadGlobalDeclAttachments(); + if (!SuccessOrErr) + return SuccessOrErr.takeError(); + assert(SuccessOrErr.get()); + // Reading the named metadata created forward references and/or // placeholders, that we flush here. resolveForwardRefsAndPlaceholders(Placeholders); @@ -2021,7 +2106,8 @@ Error MetadataLoader::MetadataLoaderImpl::parseGlobalObjectAttachment( auto K = MDKindMap.find(Record[I]); if (K == MDKindMap.end()) return error("Invalid ID"); - MDNode *MD = MetadataList.getMDNodeFwdRefOrNull(Record[I + 1]); + MDNode *MD = + dyn_cast_or_null(getMetadataFwdRefOrLoad(Record[I + 1])); if (!MD) return error("Invalid metadata attachment: expect fwd ref to MDNode"); GO.addMetadata(K->second, *MD); diff --git a/test/ThinLTO/X86/devirt2.ll b/test/ThinLTO/X86/devirt2.ll index a2cffa7be8c..71b36552353 100644 --- a/test/ThinLTO/X86/devirt2.ll +++ b/test/ThinLTO/X86/devirt2.ll @@ -15,8 +15,10 @@ ; ENABLESPLITFLAG: !{i32 1, !"EnableSplitLTOUnit", i32 1} ; Generate unsplit module with summary for ThinLTO index-based WPD. -; RUN: opt -thinlto-bc -o %t3.o %s -; RUN: opt -thinlto-bc -o %t4.o %p/Inputs/devirt2.ll +; Force generation of the bitcode index so that we also test lazy metadata +; loader handling of the type metadata. +; RUN: opt -bitcode-mdindex-threshold=0 -thinlto-bc -o %t3.o %s +; RUN: opt -bitcode-mdindex-threshold=0 -thinlto-bc -o %t4.o %p/Inputs/devirt2.ll ; Check that we don't have module flag when splitting not enabled for ThinLTO, ; and that we generate summary information needed for index-based WPD.