[ThinLTO] Add MODULE_CODE_METADATA_VALUES record

[tejohnson]

Summary

This is split out from the ThinLTO metadata mapping patch
http://reviews.llvm.org/D14752.

To avoid needing to parse the module level metadata during function
importing, a new module-level record is added which holds the
number of module-level metadata values. This is required because
metadata value ids are assigned implicitly during parsing, and the
function-level metadata ids start after the module-level metadata ids.

I made a change to this version of the code compared to D14752
in order to add more consistent and thorough assertion checking of the
new record value. We now unconditionally use the record value to
initialize the MDValueList size, and handle it the same in parseMetadata
for all module level metadata cases (lazy loading or not).

[mehdi_amini]

Looks good overall, I need to do a second pass later, probably this afternoon.

lib/Bitcode/Reader/BitcodeReader.cpp

157 ↗

(On Diff #40662)

One line comment for NumModuleMDs?

158 ↗

(On Diff #40662)

I suggest to substitute "this" with "the MODULE_CODE_METADATA_VALUES"

1922 ↗

(On Diff #40662)

Is it possible to have NextMDValueNo != 0 and SeenModuleValuesRecord? It there some redundancy here?

3292 ↗

(On Diff #40662)

This change does not seem necessary here

5351 ↗

(On Diff #40662)

remove

[tejohnson]

Thanks, comments addressed and will upload new patch in a moment.

lib/Bitcode/Reader/BitcodeReader.cpp

1922 ↗

(On Diff #40662)

Yes when ModuleLevel=true because we always initialize the MDValueList size when we read the MODULE_CODE_METADATA_VALUES record (where we set SeenModuleValuesRecord). This reset ensures that we assign MD value numbers here properly for module level metadata.

3292 ↗

(On Diff #40662)

Removed. Leftover from prior approach.

[tejohnson]

• Address Mehdi's comments.

[mehdi_amini]

LGTM (with one question)

lib/Bitcode/Reader/BitcodeReader.cpp

1924 ↗

(On Diff #40667)

I got a typo in my question, I meant: is it possible to have NextMDValueNo != 0 and *not* SeenModuleValuesRecord?

In the end my question is: do we need to check SeenModuleValuesRecord here? Is there a difference between this check and the following:

unsigned NextMDValueNo = ModuleLevel ? 0 : MDValueList.size();

(Not saying you should write it this way, but what about if (ModuleLevel) {)

llvm/trunk/include/llvm/Bitcode/LLVMBitCodes.h

enum ModuleCodes {
 
     MODULE_CODE_GCNAME      = 11,  // GCNAME: [strchr x N]
     MODULE_CODE_COMDAT      = 12,  // COMDAT: [selection_kind, name]
 
     MODULE_CODE_VSTOFFSET   = 13,  // VSTOFFSET: [offset]
 
     // ALIAS: [alias value type, addrspace, aliasee val#, linkage, visibility]
     MODULE_CODE_ALIAS       = 14,
+
+    // METADATA_VALUES: [numvals]
+    MODULE_CODE_METADATA_VALUES = 15,
   };
 
   /// PARAMATTR blocks have code for defining a parameter attribute set.
   enum AttributeCodes {
     // FIXME: Remove `PARAMATTR_CODE_ENTRY_OLD' in 4.0
     PARAMATTR_CODE_ENTRY_OLD  = 1, // ENTRY: [paramidx0, attr0,
                                    //         paramidx1, attr1...]
     PARAMATTR_CODE_ENTRY      = 2, // ENTRY: [paramidx0, attrgrp0,

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp

class BitcodeReader : public GVMaterializer {
   // Next offset to start scanning for lazy parsing of function bodies.
   uint64_t NextUnreadBit = 0;
   // Last function offset found in the VST.
   uint64_t LastFunctionBlockBit = 0;
   bool SeenValueSymbolTable = false;
   uint64_t VSTOffset = 0;
   // Contains an arbitrary and optional string identifying the bitcode producer
   std::string ProducerIdentification;
+  // Number of module level metadata records specified by the
+  // MODULE_CODE_METADATA_VALUES record.
+  unsigned NumModuleMDs = 0;
+  // Support older bitcode without the MODULE_CODE_METADATA_VALUES record.
+  bool SeenModuleValuesRecord = false;
 
   std::vector<Type*> TypeList;
   BitcodeReaderValueList ValueList;
   BitcodeReaderMDValueList MDValueList;
   std::vector<Comdat *> ComdatList;
   SmallVector<Instruction *, 64> InstructionList;
 
   std::vector<std::pair<GlobalVariable*, unsigned> > GlobalInits;
private:
   std::error_code parseConstants();
   std::error_code rememberAndSkipFunctionBodies();
   std::error_code rememberAndSkipFunctionBody();
   /// Save the positions of the Metadata blocks and skip parsing the blocks.
   std::error_code rememberAndSkipMetadata();
   std::error_code parseFunctionBody(Function *F);
   std::error_code globalCleanup();
   std::error_code resolveGlobalAndAliasInits();
-  std::error_code parseMetadata();
+  std::error_code parseMetadata(bool ModuleLevel = false);
   std::error_code parseMetadataKinds();
   std::error_code parseMetadataKindRecord(SmallVectorImpl<uint64_t> &Record);
   std::error_code parseMetadataAttachment(Function &F);
   ErrorOr<std::string> parseModuleTriple();
   std::error_code parseUseLists();
   std::error_code initStream(std::unique_ptr<DataStreamer> Streamer);
   std::error_code initStreamFromBuffer();
   std::error_code initLazyStream(std::unique_ptr<DataStreamer> Streamer);
BitcodeReader::parseMetadataKindRecord(SmallVectorImpl<uint64_t> &Record) {
   unsigned NewKind = TheModule->getMDKindID(Name.str());
   if (!MDKindMap.insert(std::make_pair(Kind, NewKind)).second)
     return error("Conflicting METADATA_KIND records");
   return std::error_code();
 }
 
 static int64_t unrotateSign(uint64_t U) { return U & 1 ? ~(U >> 1) : U >> 1; }
 
-std::error_code BitcodeReader::parseMetadata() {
+/// Parse a METADATA_BLOCK. If ModuleLevel is true then we are parsing
+/// module level metadata.
+std::error_code BitcodeReader::parseMetadata(bool ModuleLevel) {
   IsMetadataMaterialized = true;
   unsigned NextMDValueNo = MDValueList.size();
+  if (ModuleLevel && SeenModuleValuesRecord) {
+    // Now that we are parsing the module level metadata, we want to restart
+    // the numbering of the MD values, and replace temp MD created earlier
+    // with their real values. If we saw a METADATA_VALUE record then we
+    // would have set the MDValueList size to the number specified in that
+    // record, to support parsing function-level metadata first, and we need
+    // to reset back to 0 to fill the MDValueList in with the parsed module
+    // The function-level metadata parsing should have reset the MDValueList
+    // size back to the value reported by the METADATA_VALUE record, saved in
+    // NumModuleMDs.
+    assert(NumModuleMDs == MDValueList.size() &&
+           "Expected MDValueList to only contain module level values");
+    NextMDValueNo = 0;
+  }
 
   if (Stream.EnterSubBlock(bitc::METADATA_BLOCK_ID))
     return error("Invalid record");
 
   SmallVector<uint64_t, 64> Record;
 
   auto getMD =
       [&](unsigned ID) -> Metadata *{ return MDValueList.getValueFwdRef(ID); };
case bitc::METADATA_KIND: {
       // Support older bitcode files that had METADATA_KIND records in a
       // block with METADATA_BLOCK_ID.
       if (std::error_code EC = parseMetadataKindRecord(Record))
         return EC;
       break;
     }
     }
   }
+  assert((!(ModuleLevel && SeenModuleValuesRecord) ||
+          NumModuleMDs == MDValueList.size()) &&
+         "Inconsistent bitcode: METADATA_VALUES mismatch");
 #undef GET_OR_DISTINCT
 }
 
 /// Parse the metadata kinds out of the METADATA_KIND_BLOCK.
 std::error_code BitcodeReader::parseMetadataKinds() {
   if (Stream.EnterSubBlock(bitc::METADATA_KIND_BLOCK_ID))
     return error("Invalid record");
 
if (Stream.SkipBlock())
     return error("Invalid record");
   return std::error_code();
 }
 
 std::error_code BitcodeReader::materializeMetadata() {
   for (uint64_t BitPos : DeferredMetadataInfo) {
     // Move the bit stream to the saved position.
     Stream.JumpToBit(BitPos);
-    if (std::error_code EC = parseMetadata())
+    if (std::error_code EC = parseMetadata(true))
       return EC;
   }
   DeferredMetadataInfo.clear();
   return std::error_code();
 }
 
 void BitcodeReader::setStripDebugInfo() { StripDebugInfo = true; }
 
case BitstreamEntry::SubBlock:
         break;
       case bitc::METADATA_BLOCK_ID:
         if (ShouldLazyLoadMetadata && !IsMetadataMaterialized) {
           if (std::error_code EC = rememberAndSkipMetadata())
             return EC;
           break;
         }
         assert(DeferredMetadataInfo.empty() && "Unexpected deferred metadata");
-        if (std::error_code EC = parseMetadata())
+        if (std::error_code EC = parseMetadata(true))
           return EC;
         break;
       case bitc::METADATA_KIND_BLOCK_ID:
         if (std::error_code EC = parseMetadataKinds())
           return EC;
         break;
       case bitc::FUNCTION_BLOCK_ID:
         // If this is the first function body we've seen, reverse the
case bitc::MODULE_CODE_PURGEVALS:
       ValueList.shrinkTo(Record[0]);
       break;
     /// MODULE_CODE_VSTOFFSET: [offset]
     case bitc::MODULE_CODE_VSTOFFSET:
       if (Record.size() < 1)
         return error("Invalid record");
       VSTOffset = Record[0];
       break;
+    /// MODULE_CODE_METADATA_VALUES: [numvals]
+    case bitc::MODULE_CODE_METADATA_VALUES:
+      if (Record.size() < 1)
+        return error("Invalid record");
+      assert(!IsMetadataMaterialized);
+      // This record contains the number of metadata values in the module-level
+      // METADATA_BLOCK. It is used to support lazy parsing of metadata as
+      // a postpass, where we will parse function-level metadata first.
+      // This is needed because the ids of metadata are assigned implicitly
+      // based on their ordering in the bitcode, with the function-level
+      // metadata ids starting after the module-level metadata ids. Otherwise,
+      // we would have to parse the module-level metadata block to prime the
+      // MDValueList when we are lazy loading metadata during function
+      // importing. Initialize the MDValueList size here based on the
+      // record value, regardless of whether we are doing lazy metadata
+      // loading, so that we have consistent handling and assertion
+      // checking in parseMetadata for module-level metadata.
+      NumModuleMDs = Record[0];
+      SeenModuleValuesRecord = true;
+      assert(MDValueList.size() == 0);
+      MDValueList.resize(NumModuleMDs);
+      break;
     }
     Record.clear();
   }
 }
 
 /// Helper to read the header common to all bitcode files.
 static bool hasValidBitcodeHeader(BitstreamCursor &Stream) {
   // Sniff for the signature.

 
 //===----------------------------------------------------------------------===//
 // GVMaterializer implementation
 //===----------------------------------------------------------------------===//
 
 void BitcodeReader::releaseBuffer() { Buffer.release(); }
 
 std::error_code BitcodeReader::materialize(GlobalValue *GV) {
+  // In older bitcode we must materialize the metadata before parsing
+  // any functions, in order to set up the MDValueList properly.
+  if (!SeenModuleValuesRecord) {
     if (std::error_code EC = materializeMetadata())
       return EC;
+  }
 
   Function *F = dyn_cast<Function>(GV);
   // If it's not a function or is already material, ignore the request.
   if (!F || !F->isMaterializable())
     return std::error_code();
 
   DenseMap<Function*, uint64_t>::iterator DFII = DeferredFunctionInfo.find(F);
   assert(DFII != DeferredFunctionInfo.end() && "Deferred function not found!");

llvm/trunk/lib/Bitcode/Writer/BitcodeWriter.cpp

for (const GlobalAlias &A : M->aliases()) {
     Vals.push_back(getEncodedDLLStorageClass(A));
     Vals.push_back(getEncodedThreadLocalMode(A));
     Vals.push_back(A.hasUnnamedAddr());
     unsigned AbbrevToUse = 0;
     Stream.EmitRecord(bitc::MODULE_CODE_ALIAS, Vals, AbbrevToUse);
     Vals.clear();
   }
 
+  // Write a record indicating the number of module-level metadata IDs
+  // This is needed because the ids of metadata are assigned implicitly
+  // based on their ordering in the bitcode, with the function-level
+  // metadata ids starting after the module-level metadata ids. For
+  // function importing where we lazy load the metadata as a postpass,
+  // we want to avoid parsing the module-level metadata before parsing
+  // the imported functions.
+  BitCodeAbbrev *Abbv = new BitCodeAbbrev();
+  Abbv->Add(BitCodeAbbrevOp(bitc::MODULE_CODE_METADATA_VALUES));
+  Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6));
+  unsigned MDValsAbbrev = Stream.EmitAbbrev(Abbv);
+  Vals.push_back(VE.numMDs());
+  Stream.EmitRecord(bitc::MODULE_CODE_METADATA_VALUES, Vals, MDValsAbbrev);
+  Vals.clear();
+
   uint64_t VSTOffsetPlaceholder =
       WriteValueSymbolTableForwardDecl(M->getValueSymbolTable(), Stream);
   return VSTOffsetPlaceholder;
 }
 
 static uint64_t GetOptimizationFlags(const Value *V) {
   uint64_t Flags = 0;
 

llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.h

public:
   unsigned getMetadataID(const Metadata *MD) const {
     auto ID = getMetadataOrNullID(MD);
     assert(ID != 0 && "Metadata not in slotcalculator!");
     return ID - 1;
   }
   unsigned getMetadataOrNullID(const Metadata *MD) const {
     return MDValueMap.lookup(MD);
   }
+  unsigned numMDs() const { return MDs.size(); }
 
   bool hasMDString() const { return HasMDString; }
   bool hasDILocation() const { return HasDILocation; }
   bool hasGenericDINode() const { return HasGenericDINode; }
 
   bool shouldPreserveUseListOrder() const { return ShouldPreserveUseListOrder; }
 
   unsigned getTypeID(Type *T) const {

llvm/trunk/tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

default: return nullptr;
       STRINGIFY_CODE(MODULE_CODE, SECTIONNAME)
       STRINGIFY_CODE(MODULE_CODE, DEPLIB) // FIXME: Remove in 4.0
       STRINGIFY_CODE(MODULE_CODE, GLOBALVAR)
       STRINGIFY_CODE(MODULE_CODE, FUNCTION)
       STRINGIFY_CODE(MODULE_CODE, ALIAS)
       STRINGIFY_CODE(MODULE_CODE, PURGEVALS)
       STRINGIFY_CODE(MODULE_CODE, GCNAME)
       STRINGIFY_CODE(MODULE_CODE, VSTOFFSET)
+      STRINGIFY_CODE(MODULE_CODE, METADATA_VALUES)
     }
   case bitc::IDENTIFICATION_BLOCK_ID:
     switch (CodeID) {
     default:
       return nullptr;
       STRINGIFY_CODE(IDENTIFICATION_CODE, STRING)
       STRINGIFY_CODE(IDENTIFICATION_CODE, EPOCH)
     }