[codeview] Don't use the type visitor to merge types

[rnk]

Summary

This didn't do much to speed things up, but it implements a FIXME, and I
think it's a nice simplification. We don't need the record kind switch.
We're doing that ourselves.

Depends on D35394

[ruiu]

I agree that this is a nice simplification.

llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp

274

What is the meaning of this boolean varaible? Looks like you are not using its value.

[rnk]

llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp

274

It indicates if all type indices were successfully remapped. This gets passed into writeIdRecord / writeRecord, and then we decide to either write the record or map this index to LF_NOTTRANSLATED. It could be renamed AllIndicesMapped, or something.

[ruiu]

LGTM

llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp

274

AllIndicesMapped would be a good name because I expected you would eventually return Succcess from this function as you did below.

291

Can you replace auto with a concrete type?

[rnk]

• comments

llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp

 //===-- TypeStreamMerger.cpp ------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/DebugInfo/CodeView/TypeStreamMerger.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
-#include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
 #include "llvm/DebugInfo/CodeView/TypeDeserializer.h"
 #include "llvm/DebugInfo/CodeView/TypeIndex.h"
 #include "llvm/DebugInfo/CodeView/TypeIndexDiscovery.h"
 #include "llvm/DebugInfo/CodeView/TypeRecord.h"
 #include "llvm/DebugInfo/CodeView/TypeTableBuilder.h"
-#include "llvm/DebugInfo/CodeView/TypeVisitorCallbacks.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 using namespace llvm;
 using namespace llvm::codeview;
 
 namespace {
 

 /// - If the type record already exists in the destination stream, discard it
 ///   and update the type index map to forward the source type index to the
 ///   existing destination type index.
 ///
 /// As an additional complication, type stream merging actually produces two
 /// streams: an item (or IPI) stream and a type stream, as this is what is
 /// actually stored in the final PDB. We choose which records go where by
 /// looking at the record kind.
-class TypeStreamMerger : public TypeVisitorCallbacks {
+class TypeStreamMerger {
 public:
   explicit TypeStreamMerger(SmallVectorImpl<TypeIndex> &SourceToDest)
       : IndexMap(SourceToDest) {
     SourceToDest.clear();
   }
 
   static const TypeIndex Untranslated;
 
-  Error visitTypeBegin(CVType &Record) override;
-  Error visitTypeEnd(CVType &Record) override;
-
   Error mergeTypesAndIds(TypeTableBuilder &DestIds, TypeTableBuilder &DestTypes,
                          const CVTypeArray &IdsAndTypes);
   Error mergeIdRecords(TypeTableBuilder &Dest,
                        ArrayRef<TypeIndex> TypeSourceToDest,
                        const CVTypeArray &Ids);
   Error mergeTypeRecords(TypeTableBuilder &Dest, const CVTypeArray &Types);
 
 private:
   Error doit(const CVTypeArray &Types);
 
+  Error remapAllTypes(const CVTypeArray &Types);
+
+  Error remapType(const CVType &Type);
+
   void addMapping(TypeIndex Idx);
 
   bool remapTypeIndex(TypeIndex &Idx);
   bool remapItemIndex(TypeIndex &Idx);
 
-  bool remapIndices(RemappedType &Record, ArrayRef<TiReference> Refs) {
-    auto OriginalData = Record.OriginalRecord.content();
-    bool Success = true;
-    for (auto &Ref : Refs) {
-      uint32_t Offset = Ref.Offset;
-      ArrayRef<uint8_t> Bytes =
-          OriginalData.slice(Ref.Offset, sizeof(TypeIndex));
-      ArrayRef<TypeIndex> TIs(reinterpret_cast<const TypeIndex *>(Bytes.data()),
-                              Ref.Count);
-      for (auto TI : TIs) {
-        TypeIndex NewTI = TI;
-        bool ThisSuccess = (Ref.Kind == TiRefKind::IndexRef)
-                               ? remapItemIndex(NewTI)
-                               : remapTypeIndex(NewTI);
-        if (ThisSuccess && NewTI != TI)
-          Record.Mappings.emplace_back(Offset, NewTI);
-        Offset += sizeof(TypeIndex);
-        Success &= ThisSuccess;
-      }
-    }
-    return Success;
-  }
+  bool remapIndices(RemappedType &Record, ArrayRef<TiReference> Refs);
 
   bool remapIndex(TypeIndex &Idx, ArrayRef<TypeIndex> Map);
 
   size_t slotForIndex(TypeIndex Idx) const {
     assert(!Idx.isSimple() && "simple type indices have no slots");
     return Idx.getIndex() - TypeIndex::FirstNonSimpleIndex;
   }
 
   Error errorCorruptRecord() const {
     return llvm::make_error<CodeViewError>(cv_error_code::corrupt_record);
   }
 
   Error writeRecord(TypeTableBuilder &Dest, const RemappedType &Record,
                     bool RemapSuccess) {
     TypeIndex DestIdx = Untranslated;
     if (RemapSuccess)
       DestIdx = Dest.writeSerializedRecord(Record);
     addMapping(DestIdx);
     return Error::success();
   }
 
-  Error writeTypeRecord(const CVType &Record) {
-    TypeIndex DestIdx =
-        DestTypeStream->writeSerializedRecord(Record.RecordData);
-    addMapping(DestIdx);
-    return Error::success();
-  }
-
-  Error writeTypeRecord(const RemappedType &Record, bool RemapSuccess) {
-    return writeRecord(*DestTypeStream, Record, RemapSuccess);
-  }
-
-  Error writeIdRecord(const RemappedType &Record, bool RemapSuccess) {
-    return writeRecord(*DestIdStream, Record, RemapSuccess);
-  }
-
   Optional<Error> LastError;
 
   bool IsSecondPass = false;
 
   unsigned NumBadIndices = 0;
 
   TypeIndex CurIndex{TypeIndex::FirstNonSimpleIndex};
 
   TypeTableBuilder *DestIdStream = nullptr;
   TypeTableBuilder *DestTypeStream = nullptr;
 
   // If we're only mapping id records, this array contains the mapping for
   // type records.
   ArrayRef<TypeIndex> TypeLookup;
 
   /// Map from source type index to destination type index. Indexed by source
   /// type index minus 0x1000.
   SmallVectorImpl<TypeIndex> &IndexMap;
 };
 
 } // end anonymous namespace
 
 const TypeIndex TypeStreamMerger::Untranslated(SimpleTypeKind::NotTranslated);
 
-Error TypeStreamMerger::visitTypeBegin(CVType &Rec) {
-  RemappedType R(Rec);
-  SmallVector<TiReference, 32> Refs;
-  discoverTypeIndices(Rec.RecordData, Refs);
-  bool Success = remapIndices(R, Refs);
-  switch (Rec.kind()) {
+static bool isIdRecord(TypeLeafKind K) {
+  switch (K) {
   case TypeLeafKind::LF_FUNC_ID:
   case TypeLeafKind::LF_MFUNC_ID:
   case TypeLeafKind::LF_STRING_ID:
   case TypeLeafKind::LF_SUBSTR_LIST:
   case TypeLeafKind::LF_BUILDINFO:
   case TypeLeafKind::LF_UDT_SRC_LINE:
   case TypeLeafKind::LF_UDT_MOD_SRC_LINE:
-    return writeIdRecord(R, Success);
+    return true;
   default:
-    return writeTypeRecord(R, Success);
-  }
-  return Error::success();
+    return false;
   }
-
-Error TypeStreamMerger::visitTypeEnd(CVType &Rec) {
-  ++CurIndex;
-  if (!IsSecondPass)
-    assert(IndexMap.size() == slotForIndex(CurIndex) &&
-           "visitKnownRecord should add one index map entry");
-  return Error::success();
 }
 
 void TypeStreamMerger::addMapping(TypeIndex Idx) {
   if (!IsSecondPass) {
     assert(IndexMap.size() == slotForIndex(CurIndex) &&
            "visitKnownRecord should add one index map entry");
     IndexMap.push_back(Idx);
   } else {
Error TypeStreamMerger::mergeTypesAndIds(TypeTableBuilder &DestIds,
                                          TypeTableBuilder &DestTypes,
                                          const CVTypeArray &IdsAndTypes) {
   DestIdStream = &DestIds;
   DestTypeStream = &DestTypes;
   return doit(IdsAndTypes);
 }
 
 Error TypeStreamMerger::doit(const CVTypeArray &Types) {
-  // We don't want to deserialize records.  I guess this flag is poorly named,
-  // but it really means "Don't deserialize records before switching on the
-  // concrete type.
-  // FIXME: We can probably get even more speed here if we don't use the visitor
-  // pipeline here, but instead write the switch ourselves.  I don't think it
-  // would buy us much since it's already pretty fast, but it's probably worth
-  // a few cycles.
-  if (auto EC =
-          codeview::visitTypeStream(Types, *this, VDS_BytesExternal))
+  if (auto EC = remapAllTypes(Types))
     return EC;
 
   // If we found bad indices but no other errors, try doing another pass and see
   // if we can resolve the indices that weren't in the map on the first pass.
   // This may require multiple passes, but we should always make progress. MASM
   // is the only known CodeView producer that makes type streams that aren't
   // topologically sorted. The standard library contains MASM-produced objects,
   // so this is important to handle correctly, but we don't have to be too
   // efficient. MASM type streams are usually very small.
   while (!LastError && NumBadIndices > 0) {
     unsigned BadIndicesRemaining = NumBadIndices;
     IsSecondPass = true;
     NumBadIndices = 0;
     CurIndex = TypeIndex(TypeIndex::FirstNonSimpleIndex);
 
-    if (auto EC =
-            codeview::visitTypeStream(Types, *this, VDS_BytesExternal))
+    if (auto EC = remapAllTypes(Types))
       return EC;
 
     assert(NumBadIndices <= BadIndicesRemaining &&
            "second pass found more bad indices");
     if (!LastError && NumBadIndices == BadIndicesRemaining) {
       return llvm::make_error<CodeViewError>(
           cv_error_code::corrupt_record, "input type graph contains cycles");
     }
   }
 
   if (LastError)
     return std::move(*LastError);
   return Error::success();
 }
 
+Error TypeStreamMerger::remapAllTypes(const CVTypeArray &Types) {
+  for (const CVType &Type : Types)
+    if (auto EC = remapType(Type))
+      return EC;
+  return Error::success();
+}
+
+Error TypeStreamMerger::remapType(const CVType &Type) {
+  RemappedType R(Type);
+  SmallVector<TiReference, 32> Refs;
+  discoverTypeIndices(Type.RecordData, Refs);
+  bool MappedAllIndices = remapIndices(R, Refs);
+  TypeTableBuilder &Dest =
+      isIdRecord(Type.kind()) ? *DestIdStream : *DestTypeStream;
+  if (auto EC = writeRecord(Dest, R, MappedAllIndices))
+    return EC;
+
+  ++CurIndex;
+  assert((IsSecondPass || IndexMap.size() == slotForIndex(CurIndex)) &&
+         "visitKnownRecord should add one index map entry");
+  return Error::success();
+}
+
+bool TypeStreamMerger::remapIndices(RemappedType &Record,
+                                    ArrayRef<TiReference> Refs) {
+  ArrayRef<uint8_t> OriginalData = Record.OriginalRecord.content();
+  bool Success = true;
+  for (auto &Ref : Refs) {
+    uint32_t Offset = Ref.Offset;
+    ArrayRef<uint8_t> Bytes = OriginalData.slice(Ref.Offset, sizeof(TypeIndex));
+    ArrayRef<TypeIndex> TIs(reinterpret_cast<const TypeIndex *>(Bytes.data()),
+                            Ref.Count);
+    for (auto TI : TIs) {
+      TypeIndex NewTI = TI;
+      bool ThisSuccess = (Ref.Kind == TiRefKind::IndexRef)
+                             ? remapItemIndex(NewTI)
+                             : remapTypeIndex(NewTI);
+      if (ThisSuccess && NewTI != TI)
+        Record.Mappings.emplace_back(Offset, NewTI);
+      Offset += sizeof(TypeIndex);
+      Success &= ThisSuccess;
+    }
+  }
+  return Success;
+}
+
 Error llvm::codeview::mergeTypeRecords(TypeTableBuilder &Dest,
                                        SmallVectorImpl<TypeIndex> &SourceToDest,
                                        const CVTypeArray &Types) {
   TypeStreamMerger M(SourceToDest);
   return M.mergeTypeRecords(Dest, Types);
 }
 
 Error llvm::codeview::mergeIdRecords(TypeTableBuilder &Dest,