[WebAssembly] Identify COMDATs by index rather than string. NFC

[ncw]

Summary

This will enable an optimisation in LLD.

[sbc100]

Nice! LVGTM

lib/Object/WasmObjectFile.cpp

928 ↗

(On Diff #137878)

I feel like make comment here might make sense. Something like "The rest of these fields get set in the linking metadata section"?

tools/obj2yaml/wasm2yaml.cpp

69 ↗

(On Diff #137878)

Should this be a little more strict? Like if (Func.Comdat != UINT32_MAX) more explicitly expressed the indent no?

84 ↗

(On Diff #137878)

ditto

llvm/trunk/include/llvm/BinaryFormat/Wasm.h

 
 struct WasmFunction {
   uint32_t Index;
   std::vector<WasmLocalDecl> Locals;
   ArrayRef<uint8_t> Body;
   uint32_t CodeSectionOffset;
   uint32_t Size;
   StringRef Name; // from the "linking" or "names" section
-  StringRef Comdat; // from the "comdat info" section
+  uint32_t Comdat; // from the "comdat info" section
 };
 
 struct WasmDataSegment {
   uint32_t MemoryIndex;
   WasmInitExpr Offset;
   ArrayRef<uint8_t> Content;
   StringRef Name;
   uint32_t Alignment;
   uint32_t Flags;
-  StringRef Comdat; // from the "comdat info" section
+  uint32_t Comdat; // from the "comdat info" section
 };
 
 struct WasmElemSegment {
   uint32_t TableIndex;
   WasmInitExpr Offset;
   std::vector<uint32_t> Functions;
 };
 

 
 struct WasmFunctionName {
   uint32_t Index;
   StringRef Name;
 };
 
 struct WasmLinkingData {
   std::vector<WasmInitFunc> InitFunctions;
+  std::vector<StringRef> Comdats;
   std::vector<WasmSymbolInfo> SymbolTable;
 };
 
 enum : unsigned {
   WASM_SEC_CUSTOM = 0,   // Custom / User-defined section
   WASM_SEC_TYPE = 1,     // Function signature declarations
   WASM_SEC_IMPORT = 2,   // Import declarations
   WASM_SEC_FUNCTION = 3, // Function declarations

llvm/trunk/include/llvm/Object/Wasm.h

public:
   ArrayRef<wasm::WasmGlobal> globals() const { return Globals; }
   ArrayRef<wasm::WasmExport> exports() const { return Exports; }
   ArrayRef<WasmSymbol> syms() const { return Symbols; }
   const wasm::WasmLinkingData& linkingData() const { return LinkingData; }
   uint32_t getNumberOfSymbols() const { return Symbols.size(); }
   ArrayRef<wasm::WasmElemSegment> elements() const { return ElemSegments; }
   ArrayRef<WasmSegment> dataSegments() const { return DataSegments; }
   ArrayRef<wasm::WasmFunction> functions() const { return Functions; }
-  ArrayRef<StringRef> comdats() const { return Comdats; }
   ArrayRef<wasm::WasmFunctionName> debugNames() const { return DebugNames; }
   uint32_t startFunction() const { return StartFunction; }
   uint32_t getNumImportedGlobals() const { return NumImportedGlobals; }
   uint32_t getNumImportedFunctions() const { return NumImportedFunctions; }
 
   void moveSymbolNext(DataRefImpl &Symb) const override;
 
   uint32_t getSymbolFlags(DataRefImpl Symb) const override;
private:
   std::vector<wasm::WasmLimits> Memories;
   std::vector<wasm::WasmGlobal> Globals;
   std::vector<wasm::WasmImport> Imports;
   std::vector<wasm::WasmExport> Exports;
   std::vector<wasm::WasmElemSegment> ElemSegments;
   std::vector<WasmSegment> DataSegments;
   std::vector<wasm::WasmFunction> Functions;
   std::vector<WasmSymbol> Symbols;
-  std::vector<StringRef> Comdats;
   std::vector<wasm::WasmFunctionName> DebugNames;
   uint32_t StartFunction = -1;
   bool HasLinkingSection = false;
   wasm::WasmLinkingData LinkingData;
   uint32_t NumImportedGlobals = 0;
   uint32_t NumImportedFunctions = 0;
   uint32_t CodeSection = 0;
   uint32_t DataSection = 0;

llvm/trunk/lib/Object/WasmObjectFile.cpp

Error WasmObjectFile::parseLinkingSectionSymtab(const uint8_t *&Ptr,
   return Error::success();
 }
 
 Error WasmObjectFile::parseLinkingSectionComdat(const uint8_t *&Ptr,
                                                 const uint8_t *End)
 {
   uint32_t ComdatCount = readVaruint32(Ptr);
   StringSet<> ComdatSet;
-  while (ComdatCount--) {
+  for (unsigned ComdatIndex = 0; ComdatIndex < ComdatCount; ++ComdatIndex) {
     StringRef Name = readString(Ptr);
     if (Name.empty() || !ComdatSet.insert(Name).second)
       return make_error<GenericBinaryError>("Bad/duplicate COMDAT name " + Twine(Name),
                                             object_error::parse_failed);
-    Comdats.emplace_back(Name);
+    LinkingData.Comdats.emplace_back(Name);
     uint32_t Flags = readVaruint32(Ptr);
     if (Flags != 0)
       return make_error<GenericBinaryError>("Unsupported COMDAT flags",
                                             object_error::parse_failed);
 
     uint32_t EntryCount = readVaruint32(Ptr);
     while (EntryCount--) {
       unsigned Kind = readVaruint32(Ptr);
       unsigned Index = readVaruint32(Ptr);
       switch (Kind) {
       default:
         return make_error<GenericBinaryError>("Invalid COMDAT entry type",
                                               object_error::parse_failed);
       case wasm::WASM_COMDAT_DATA:
         if (Index >= DataSegments.size())
           return make_error<GenericBinaryError>("COMDAT data index out of range",
                                                 object_error::parse_failed);
-        if (!DataSegments[Index].Data.Comdat.empty())
+        if (DataSegments[Index].Data.Comdat != UINT32_MAX)
           return make_error<GenericBinaryError>("Data segment in two COMDATs",
                                                 object_error::parse_failed);
-        DataSegments[Index].Data.Comdat = Name;
+        DataSegments[Index].Data.Comdat = ComdatIndex;
         break;
       case wasm::WASM_COMDAT_FUNCTION:
         if (!isDefinedFunctionIndex(Index))
           return make_error<GenericBinaryError>("COMDAT function index out of range",
                                                 object_error::parse_failed);
-        if (!getDefinedFunction(Index).Comdat.empty())
+        if (getDefinedFunction(Index).Comdat != UINT32_MAX)
           return make_error<GenericBinaryError>("Function in two COMDATs",
                                                 object_error::parse_failed);
-        getDefinedFunction(Index).Comdat = Name;
+        getDefinedFunction(Index).Comdat = ComdatIndex;
         break;
       }
     }
   }
   return Error::success();
 }
 
 WasmSection* WasmObjectFile::findCustomSectionByName(StringRef Name) {
while (NumLocalDecls--) {
       wasm::WasmLocalDecl Decl;
       Decl.Count = readVaruint32(Ptr);
       Decl.Type = readUint8(Ptr);
       Function.Locals.push_back(Decl);
     }
 
     uint32_t BodySize = FunctionEnd - Ptr;
     Function.Body = ArrayRef<uint8_t>(Ptr, BodySize);
+    // This will be set later when reading in the linking metadata section.
+    Function.Comdat = UINT32_MAX;
     Ptr += BodySize;
     assert(Ptr == FunctionEnd);
     Functions.push_back(Function);
   }
   if (Ptr != End)
     return make_error<GenericBinaryError>("Code section ended prematurely",
                                           object_error::parse_failed);
   return Error::success();
Error WasmObjectFile::parseDataSection(const uint8_t *Ptr, const uint8_t *End) {
   DataSegments.reserve(Count);
   while (Count--) {
     WasmSegment Segment;
     Segment.Data.MemoryIndex = readVaruint32(Ptr);
     if (Error Err = readInitExpr(Segment.Data.Offset, Ptr))
       return Err;
     uint32_t Size = readVaruint32(Ptr);
     Segment.Data.Content = ArrayRef<uint8_t>(Ptr, Size);
+    // The rest of these Data fields are set later, when reading in the linking
+    // metadata section.
     Segment.Data.Alignment = 0;
     Segment.Data.Flags = 0;
+    Segment.Data.Comdat = UINT32_MAX;
     Segment.SectionOffset = Ptr - Start;
     Ptr += Size;
     DataSegments.push_back(Segment);
   }
   if (Ptr != End)
     return make_error<GenericBinaryError>("Data section ended prematurely",
                                           object_error::parse_failed);
   return Error::success();

llvm/trunk/tools/obj2yaml/wasm2yaml.cpp

for (const llvm::wasm::WasmFunctionName &Func: Obj.debugNames()) {
       WasmYAML::NameEntry NameEntry;
       NameEntry.Name = Func.Name;
       NameEntry.Index = Func.Index;
       NameSec->FunctionNames.push_back(NameEntry);
     }
     CustomSec = std::move(NameSec);
   } else if (WasmSec.Name == "linking") {
     std::unique_ptr<WasmYAML::LinkingSection> LinkingSec = make_unique<WasmYAML::LinkingSection>();
-    std::map<StringRef,size_t> ComdatIndexes;
-    for (StringRef ComdatName : Obj.comdats()) {
-      ComdatIndexes[ComdatName] = LinkingSec->Comdats.size();
+    ArrayRef<StringRef> Comdats = Obj.linkingData().Comdats;
+    for (StringRef ComdatName : Comdats)
       LinkingSec->Comdats.emplace_back(WasmYAML::Comdat{ComdatName, {}});
-    }
     for (auto &Func : Obj.functions()) {
-      if (!Func.Comdat.empty()) {
-        auto &Comdat = LinkingSec->Comdats[ComdatIndexes[Func.Comdat]];
-        Comdat.Entries.emplace_back(
+      if (Func.Comdat != UINT32_MAX) {
+        LinkingSec->Comdats[Func.Comdat].Entries.emplace_back(
                 WasmYAML::ComdatEntry{wasm::WASM_COMDAT_FUNCTION, Func.Index});
       }
     }
     uint32_t SegmentIndex = 0;
     for (const object::WasmSegment &Segment : Obj.dataSegments()) {
       if (!Segment.Data.Name.empty()) {
         WasmYAML::SegmentInfo SegmentInfo;
         SegmentInfo.Name = Segment.Data.Name;
         SegmentInfo.Index = SegmentIndex;
         SegmentInfo.Alignment = Segment.Data.Alignment;
         SegmentInfo.Flags = Segment.Data.Flags;
         LinkingSec->SegmentInfos.push_back(SegmentInfo);
       }
-      if (!Segment.Data.Comdat.empty()) {
-        auto &Comdat = LinkingSec->Comdats[ComdatIndexes[Segment.Data.Comdat]];
-        Comdat.Entries.emplace_back(
+      if (Segment.Data.Comdat != UINT32_MAX) {
+        LinkingSec->Comdats[Segment.Data.Comdat].Entries.emplace_back(
             WasmYAML::ComdatEntry{wasm::WASM_COMDAT_DATA, SegmentIndex});
       }
       SegmentIndex++;
     }
     uint32_t SymbolIndex = 0;
     for (const wasm::WasmSymbolInfo &Symbol : Obj.linkingData().SymbolTable) {
       WasmYAML::SymbolInfo Info;
       Info.Index = SymbolIndex++;