[llvm] 01b9deb - Revert D87970 "[ThinLTO] Avoid temporaries when loading global decl attachment metadata"
Fangrui Song via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 23 10:24:16 PDT 2020
Author: Fangrui Song
Date: 2020-09-23T10:24:08-07:00
New Revision: 01b9deba76a950f04574b656c7c31ae389104f2d
URL: https://github.com/llvm/llvm-project/commit/01b9deba76a950f04574b656c7c31ae389104f2d
DIFF: https://github.com/llvm/llvm-project/commit/01b9deba76a950f04574b656c7c31ae389104f2d.diff
LOG: Revert D87970 "[ThinLTO] Avoid temporaries when loading global decl attachment metadata"
This reverts commit ab1b4810b55279bcf6fdd87be74a403440be3991.
It caused an issue in llvm::lto::thinBackend for a -fsanitize=cfi build.
```
AbbrevNo is 0 => "Invalid abbrev number"
0 llvm::BitstreamCursor::getAbbrev (this=0x9db4c8, AbbrevID=4) at llvm/include/llvm/Bitstream/BitstreamReader.h:528
1 0x00007f5f777a6eb4 in llvm::BitstreamCursor::readRecord (this=0x9db4c8, AbbrevID=4, Vals=llvm::SmallVector of Size 0, Capacity 64, Blob=0x7ffcd0e26558) at
usr/local/google/home/maskray/llvm/llvm/lib/Bitstream/Reader/BitstreamReader.cpp:228
2 0x00007f5f796bf633 in llvm::MetadataLoader::MetadataLoaderImpl::lazyLoadOneMetadata (this=0x9db3a0, ID=188, Placeholders=...) at /usr/local/google/home/mas
ray/llvm/llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1091
3 0x00007f5f796c2527 in llvm::MetadataLoader::MetadataLoaderImpl::getMetadataFwdRefOrLoad (this=0x9db3a0, ID=188) at llvm
lib/Bitcode/Reader/MetadataLoader.cpp:668
4 0x00007f5f796bfff3 in llvm::MetadataLoader::getMetadataFwdRefOrLoad (this=0xd31580, Idx=188) at llvm/lib/Bitcode/Reader
MetadataLoader.cpp:2290
5 0x00007f5f79638265 in (anonymous namespace)::BitcodeReader::parseFunctionBody (this=0xd312e0, F=0x9de758) at llvm/lib/B
tcode/Reader/BitcodeReader.cpp:3938
6 0x00007f5f79635d32 in (anonymous namespace)::BitcodeReader::materialize (this=0xd312e0, GV=0x9de758) at llvm/lib/Bitcod
/Reader/BitcodeReader.cpp:5408
7 0x00007f5f7f8dbe3e in llvm::Module::materialize (this=0x9b92c0, GV=0x9de758) at llvm/lib/IR/Module.cpp:442
8 0x00007f5f7f7f8fbe in llvm::GlobalValue::materialize (this=0x9de758) at llvm/lib/IR/Globals.cpp:50
9 0x00007f5f83b9b5f5 in llvm::FunctionImporter::importFunctions (this=0x7ffcd0e2a730, DestModule=..., ImportList=...) at
llvm/lib/Transforms/IPO/FunctionImport.cpp:1182
```
Added:
Modified:
llvm/lib/Bitcode/Reader/MetadataLoader.cpp
llvm/test/ThinLTO/X86/devirt2.ll
Removed:
################################################################################
diff --git a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
index 01ebad9594ec..874bb84170df 100644
--- a/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
+++ b/llvm/lib/Bitcode/Reader/MetadataLoader.cpp
@@ -438,20 +438,6 @@ class MetadataLoader::MetadataLoaderImpl {
/// Index that keeps track of where to find a metadata record in the stream.
std::vector<uint64_t> 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<bool> loadGlobalDeclAttachments();
-
/// Populate the index above to enable lazily loading of metadata, and load
/// the named metadata as well as the transitively referenced global
/// Metadata.
@@ -695,10 +681,8 @@ Expected<bool>
MetadataLoader::MetadataLoaderImpl::lazyLoadModuleMetadataBlock() {
IndexCursor = Stream;
SmallVector<uint64_t, 64> Record;
- GlobalDeclAttachmentPos = 0;
// Get the abbrevs, and preload record positions to make them lazy-loadable.
while (true) {
- uint64_t SavedPos = IndexCursor.GetCurrentBitNo();
Expected<BitstreamEntry> MaybeEntry = IndexCursor.advanceSkippingSubblocks(
BitstreamCursor::AF_DontPopBlockAtEnd);
if (!MaybeEntry)
@@ -833,11 +817,25 @@ MetadataLoader::MetadataLoaderImpl::lazyLoadModuleMetadataBlock() {
break;
}
case bitc::METADATA_GLOBAL_DECL_ATTACHMENT: {
- if (!GlobalDeclAttachmentPos)
- GlobalDeclAttachmentPos = SavedPos;
-#ifndef NDEBUG
- NumGlobalDeclAttachSkipped++;
-#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<unsigned> 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<GlobalObject>(ValueList[ValueID]))
+ if (Error Err = parseGlobalObjectAttachment(
+ *GO, ArrayRef<uint64_t>(Record).slice(1)))
+ return std::move(Err);
break;
}
case bitc::METADATA_KIND:
@@ -887,81 +885,6 @@ 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<bool> MetadataLoader::MetadataLoaderImpl::loadGlobalDeclAttachments() {
- // Nothing to do if we didn't find any of these metadata records.
- if (!GlobalDeclAttachmentPos)
- return true;
- IndexCursor = Stream;
- SmallVector<uint64_t, 64> 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<BitstreamEntry> 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<unsigned> 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<unsigned> 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<GlobalObject>(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<uint64_t>(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) {
@@ -991,14 +914,6 @@ 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);
@@ -2106,8 +2021,7 @@ Error MetadataLoader::MetadataLoaderImpl::parseGlobalObjectAttachment(
auto K = MDKindMap.find(Record[I]);
if (K == MDKindMap.end())
return error("Invalid ID");
- MDNode *MD =
- dyn_cast_or_null<MDNode>(getMetadataFwdRefOrLoad(Record[I + 1]));
+ MDNode *MD = MetadataList.getMDNodeFwdRefOrNull(Record[I + 1]);
if (!MD)
return error("Invalid metadata attachment: expect fwd ref to MDNode");
GO.addMetadata(K->second, *MD);
diff --git a/llvm/test/ThinLTO/X86/devirt2.ll b/llvm/test/ThinLTO/X86/devirt2.ll
index 71b36552353f..a2cffa7be8c3 100644
--- a/llvm/test/ThinLTO/X86/devirt2.ll
+++ b/llvm/test/ThinLTO/X86/devirt2.ll
@@ -15,10 +15,8 @@
; ENABLESPLITFLAG: !{i32 1, !"EnableSplitLTOUnit", i32 1}
; Generate unsplit module with summary for ThinLTO index-based WPD.
-; 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
+; RUN: opt -thinlto-bc -o %t3.o %s
+; RUN: opt -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.
More information about the llvm-commits
mailing list