Use Blob to record MDString

[mehdi_amini]

Summary

This makes encoding/decoding much simpler/faster. This is taking
a bit more space because of alignment over the current encoding
which is already fixed 8-bits.

• llvm-bcanalyzer reports on a RelWithDebInfo compile of DAGISelEmitter.cpp that the space occupied by the MDString goes from 12134612b to 12586813b (+3.7%). The total file goes from 18513491b to 18965683b (+2.4%).
• MaterializeMetadata (during ThinLTO-link of llvm-tblgen) goes from 8.2s to 6.4s (-24.4%).

[tejohnson]

Is this obsolete with Duncan's change?

[mehdi_amini]

Yes

include/llvm/Support/StreamingMemoryObject.h

 //===- StreamingMemoryObject.h - Streamable data interface -----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_STREAMINGMEMORYOBJECT_H
 #define LLVM_SUPPORT_STREAMINGMEMORYOBJECT_H
 
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/DataStream.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MemoryObject.h"
 #include <memory>
 #include <vector>
 
 namespace llvm {
 
 /// Interface to data which is actually streamed from a DataStreamer. In
 /// addition to inherited members, it has the dropLeadingBytes and
 /// setKnownObjectSize methods which are not applicable to non-streamed objects.
 class StreamingMemoryObject : public MemoryObject {
+  /// Buffer for temporary storage if getPointer() is used
+  mutable SmallVector<uint8_t, 128> Buffer;
+
 public:
   StreamingMemoryObject(std::unique_ptr<DataStreamer> Streamer);
   uint64_t getExtent() const override;
   uint64_t readBytes(uint8_t *Buf, uint64_t Size,
                      uint64_t Address) const override;
-  const uint8_t *getPointer(uint64_t address, uint64_t size) const override {
-    // FIXME: This could be fixed by ensuring the bytes are fetched and
-    // making a copy, requiring that the bitcode size be known, or
-    // otherwise ensuring that the memory doesn't go away/get reallocated,
-    // but it's not currently necessary. Users that need the pointer (any
-    // that need Blobs) don't stream.
-    report_fatal_error("getPointer in streaming memory objects not allowed");
-    return nullptr;
+  const uint8_t *getPointer(uint64_t Address, uint64_t Size) const override {
+    Buffer.resize(Size);
+    readBytes(&Buffer[0], Size, Address);
+    return &Buffer[0];
   }
   bool isValidAddress(uint64_t address) const override;
 
   /// Drop s bytes from the front of the stream, pushing the positions of the
   /// remaining bytes down by s. This is used to skip past the bitcode header,
   /// since we don't know a priori if it's present, and we can't put bytes
   /// back into the stream once we've read them.
   bool dropLeadingBytes(size_t s);

lib/Bitcode/Reader/BitcodeReader.cpp

case BitstreamEntry::EndBlock:
              "Inconsistent bitcode: METADATA_VALUES mismatch");
       return std::error_code();
     case BitstreamEntry::Record:
       // The interesting case.
       break;
     }
 
     // Read a record.
+    StringRef Blob; // Optional blob that readRecord can supply.
     Record.clear();
-    unsigned Code = Stream.readRecord(Entry.ID, Record);
+    unsigned Code = Stream.readRecord(Entry.ID, Record, &Blob);
     bool IsDistinct = false;
+    assert((Blob.empty() || Code == bitc::METADATA_STRING) &&
+           "Unexpected Blob for non-METADATA_STRING record");
     switch (Code) {
     default:  // Default behavior: ignore.
       break;
     case bitc::METADATA_NAME: {
       // Read name of the named metadata.
       SmallString<8> Name(Record.begin(), Record.end());
       Record.clear();
       Code = Stream.ReadCode();
case bitc::METADATA_IMPORTED_ENTITY: {
           GET_OR_DISTINCT(DIImportedEntity, Record[0],
                           (Context, Record[1], getMDOrNull(Record[2]),
                            getMDOrNull(Record[3]), Record[4],
                            getMDString(Record[5]))),
           NextMetadataNo++);
       break;
     }
     case bitc::METADATA_STRING: {
+      if (!Blob.empty()) {
+        Metadata *MD = MDString::get(Context, Blob);
+        MetadataList.assignValue(MD, NextMetadataNo++);
+      } else {
+        // Old bitcode didn't use blob here.
         std::string String(Record.begin(), Record.end());
         llvm::UpgradeMDStringConstant(String);
         Metadata *MD = MDString::get(Context, String);
         MetadataList.assignValue(MD, NextMetadataNo++);
+      }
       break;
     }
     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;

lib/Bitcode/Writer/BitcodeWriter.cpp

static void WriteModuleMetadata(const Module *M,
 
   Stream.EnterSubblock(bitc::METADATA_BLOCK_ID, 3);
 
   unsigned MDSAbbrev = 0;
   if (VE.hasMDString()) {
     // Abbrev for METADATA_STRING.
     BitCodeAbbrev *Abbv = new BitCodeAbbrev();
     Abbv->Add(BitCodeAbbrevOp(bitc::METADATA_STRING));
-    Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Array));
-    Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 8));
+    Abbv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob));
     MDSAbbrev = Stream.EmitAbbrev(Abbv);
   }
 
   // Initialize MDNode abbreviations.
 #define HANDLE_MDNODE_LEAF(CLASS) unsigned CLASS##Abbrev = 0;
 #include "llvm/IR/Metadata.def"
 
   if (VE.hasDILocation()) {

test/Bitcode/invalid.test

 
 INVALID-CODELENWIDTH: Malformed block
 
 RUN: not llvm-dis -disable-output %p/Inputs/invalid-function-argument-type.bc 2>&1 | \
 RUN:   FileCheck --check-prefix=INVALID-ARGUMENT-TYPE %s
 
 INVALID-ARGUMENT-TYPE: Invalid function argument type
 
-RUN: not llvm-dis -disable-output %p/Inputs/invalid-fixme-streaming-blob.bc 2>&1 | \
-RUN:   FileCheck --check-prefix=STREAMING-BLOB %s
-
-STREAMING-BLOB: getPointer in streaming memory objects not allowed
-
 RUN: not llvm-dis -disable-output %p/Inputs/invalid-function-comdat-id.bc 2>&1 | \
 RUN:   FileCheck --check-prefix=INVALID-FCOMDAT-ID %s
 
 INVALID-FCOMDAT-ID: Invalid function comdat ID
 
 RUN: not llvm-dis -disable-output %p/Inputs/invalid-global-var-comdat-id.bc 2>&1 | \
 RUN:   FileCheck --check-prefix=INVALID-GVCOMDAT-ID %s