[WebAssembly] Simplify initializeSymbols and merge it with ObjFile::parse. NFC.

[ruiu]

Summary

This patch simplifies initializeSymbols. Since that function is called
at the tail context of ObjFile::parse, and the function is called only
once from that function, that's effectively just a continuation of
ObjFile::parse. So this patch merge it with ObjFile::parse.

[sbc100]

My comments only relate to the removal of the members and other refactoring. Maybe you want land this without those extra parts and make that a separate change?

lld/wasm/InputFiles.cpp

46 ↗

(On Diff #136207)

Call this NumFuncs is a little deceptive since its the number of imported functions, which is doesn't include the defined functions.

Perhaps just inline these calls below to avoid the variable?

168 ↗

(On Diff #136207)

This reads backwards to me.

I'd prefer it to read "Assert that the symbol's element index is less than the number of imported functions"... which is the same thing as saying "Assert that the symbol points to an imported function rather than a defined one".

Why not keep the more meaningful "NumFunctionImports" name? Or perhaps just call it twice since the first call is inside of an assert (so won't effect release performance)?

[ruiu]

lld/wasm/InputFiles.cpp

168 ↗

(On Diff #136207)

The reason why I removed these member variables is because they don't do anything expensive and actually as cheap as member variables. They are defined as shown below.

uint32_t getNumImportedGlobals() const { return NumImportedGlobals; }
uint32_t getNumImportedFunctions() const { return NumImportedFunctions; }

So, what "caching" results of these functions does is to make extra copies of these member variables in our instances, which doesn't make much sense to me. I'm not worried about the cost of having extra copies, but if we have two copies of the same data, that is error prone.

[ruiu]

lld/wasm/InputFiles.cpp

168 ↗

(On Diff #136207)

Oh sorry I misread your comment. To me

uint32_t N = WasmObj->getNumImportedFunctions();
assert(Sym.Info.ElementIndex >= N);
return Functions[Sym.Info.ElementIndex - N];

is easier to read than

assert(Sym.Info.ElementIndex >= WasmObj->getNumImportedFunctions());
return Functions[Sym.Info.ElementIndex - WasmObj->getNumImportedFunctions()];

because the latter is longer, and it isn't obvious to me that the two function calls are actually the same.

The reason why I wrote N <= something instead of something >= N is because I'm always thinking on the number line. N is left if it is smaller than the other.

But that's just my preference. If you feel strongly about it I can revert it.

[ruiu]

lld/wasm/InputFiles.cpp

168 ↗

(On Diff #136207)

On second thought, I think we don't really need this assert. If assertion is enabled, out of bound should be caught by the container, and no explicit bound checking isn't needed.

[ruiu]

• inline get{Global,Data,Function}Symbol functions

[ruiu]

• update dumpInfo

[sbc100]

You are right, I misread the comparison. LGTM.

lld/trunk/wasm/InputFiles.h

public:
   FunctionSymbol *getFunctionSymbol(uint32_t Index) const;
   DataSymbol *getDataSymbol(uint32_t Index) const;
   GlobalSymbol *getGlobalSymbol(uint32_t Index) const;
 
 private:
   Symbol *createDefined(const WasmSymbol &Sym);
   Symbol *createUndefined(const WasmSymbol &Sym);
 
-  void initializeSymbols();
-  InputSegment *getSegment(const WasmSymbol &WasmSym) const;
-  InputFunction *getFunction(const WasmSymbol &Sym) const;
-  InputGlobal *getGlobal(const WasmSymbol &Sym) const;
   bool isExcludedByComdat(InputChunk *Chunk) const;
 
   // List of all symbols referenced or defined by this file.
   std::vector<Symbol *> Symbols;
 
-  uint32_t NumGlobalImports = 0;
-  uint32_t NumFunctionImports = 0;
   std::unique_ptr<WasmObjectFile> WasmObj;
 };
 
 // Opens a given file.
 llvm::Optional<MemoryBufferRef> readFile(StringRef Path);
 
 } // namespace wasm
 
 std::string toString(const wasm::InputFile *File);
 
 } // namespace lld
 
 #endif

lld/trunk/wasm/InputFiles.cpp

Optional<MemoryBufferRef> lld::wasm::readFile(StringRef Path) {
   std::unique_ptr<MemoryBuffer> &MB = *MBOrErr;
   MemoryBufferRef MBRef = MB->getMemBufferRef();
   make<std::unique_ptr<MemoryBuffer>>(std::move(MB)); // take MB ownership
 
   return MBRef;
 }
 
 void ObjFile::dumpInfo() const {
-  log("info for: " + getName() + "\n" +
-      "              Symbols : " + Twine(Symbols.size()) + "\n" +
-      "     Function Imports : " + Twine(NumFunctionImports) + "\n" +
-      "       Global Imports : " + Twine(NumGlobalImports) + "\n");
+  log("info for: " + getName() +
+      "\n              Symbols : " + Twine(Symbols.size()) +
+      "\n     Function Imports : " + Twine(WasmObj->getNumImportedFunctions()) +
+      "\n       Global Imports : " + Twine(WasmObj->getNumImportedGlobals()));
 }
 
 // Relocations contain an index into the function, global or table index
 // space of the input file.  This function takes a relocation and returns the
 // relocated index (i.e. translates from the input index space to the output
 // index space).
 uint32_t ObjFile::calcNewIndex(const WasmRelocation &Reloc) const {
   if (Reloc.Type == R_WEBASSEMBLY_TYPE_INDEX_LEB) {
if (Section.Type == WASM_SEC_CODE)
       CodeSection = &Section;
     else if (Section.Type == WASM_SEC_DATA)
       DataSection = &Section;
   }
 
   TypeMap.resize(getWasmObj()->types().size());
   TypeIsUsed.resize(getWasmObj()->types().size(), false);
 
-  initializeSymbols();
-}
+  for (StringRef Name : WasmObj->comdats())
+    Symtab->addComdat(Name, this);
 
-// Return the InputSegment in which a given symbol is defined.
-InputSegment *ObjFile::getSegment(const WasmSymbol &WasmSym) const {
-  return Segments[WasmSym.Info.DataRef.Segment];
+  // Populate `Segments`.
+  for (const WasmSegment &S : WasmObj->dataSegments()) {
+    InputSegment *Seg = make<InputSegment>(S, this);
+    Seg->copyRelocations(*DataSection);
+    Segments.emplace_back(Seg);
   }
 
-InputFunction *ObjFile::getFunction(const WasmSymbol &Sym) const {
-  assert(Sym.Info.ElementIndex >= NumFunctionImports);
-  uint32_t FunctionIndex = Sym.Info.ElementIndex - NumFunctionImports;
-  return Functions[FunctionIndex];
+  // Populate `Functions`.
+  ArrayRef<WasmFunction> Funcs = WasmObj->functions();
+  ArrayRef<uint32_t> FuncTypes = WasmObj->functionTypes();
+  ArrayRef<WasmSignature> Types = WasmObj->types();
+  Functions.reserve(Funcs.size());
+
+  for (size_t I = 0, E = Funcs.size(); I != E; ++I) {
+    InputFunction *F =
+        make<InputFunction>(Types[FuncTypes[I]], &Funcs[I], this);
+    F->copyRelocations(*CodeSection);
+    Functions.emplace_back(F);
   }
 
-InputGlobal *ObjFile::getGlobal(const WasmSymbol &Sym) const {
-  assert(Sym.Info.ElementIndex >= NumGlobalImports);
-  uint32_t GlobalIndex = Sym.Info.ElementIndex - NumGlobalImports;
-  return Globals[GlobalIndex];
+  // Populate `Globals`.
+  for (const WasmGlobal &G : WasmObj->globals())
+    Globals.emplace_back(make<InputGlobal>(G));
+
+  // Populate `Symbols` based on the WasmSymbols in the object.
+  Symbols.reserve(WasmObj->getNumberOfSymbols());
+  for (const SymbolRef &Sym : WasmObj->symbols()) {
+    const WasmSymbol &WasmSym = WasmObj->getWasmSymbol(Sym.getRawDataRefImpl());
+    if (Symbol *Sym = createDefined(WasmSym))
+      Symbols.push_back(Sym);
+    else
+      Symbols.push_back(createUndefined(WasmSym));
+  }
 }
 
 bool ObjFile::isExcludedByComdat(InputChunk *Chunk) const {
   StringRef Comdat = Chunk->getComdat();
   return !Comdat.empty() && Symtab->findComdat(Comdat) != this;
 }
 
 FunctionSymbol *ObjFile::getFunctionSymbol(uint32_t Index) const {
   return cast<FunctionSymbol>(Symbols[Index]);
 }
 
 GlobalSymbol *ObjFile::getGlobalSymbol(uint32_t Index) const {
   return cast<GlobalSymbol>(Symbols[Index]);
 }
 
 DataSymbol *ObjFile::getDataSymbol(uint32_t Index) const {
   return cast<DataSymbol>(Symbols[Index]);
 }
 
-void ObjFile::initializeSymbols() {
-  Symbols.reserve(WasmObj->getNumberOfSymbols());
-
-  NumFunctionImports = WasmObj->getNumImportedFunctions();
-  NumGlobalImports = WasmObj->getNumImportedGlobals();
-
-  ArrayRef<WasmFunction> Funcs = WasmObj->functions();
-  ArrayRef<uint32_t> FuncTypes = WasmObj->functionTypes();
-  ArrayRef<WasmSignature> Types = WasmObj->types();
-
-  for (StringRef Name : WasmObj->comdats())
-    Symtab->addComdat(Name, this);
-
-  for (const WasmSegment &S : WasmObj->dataSegments()) {
-    InputSegment *Seg = make<InputSegment>(S, this);
-    Seg->copyRelocations(*DataSection);
-    Segments.emplace_back(Seg);
-  }
-
-  for (const WasmGlobal &G : WasmObj->globals())
-    Globals.emplace_back(make<InputGlobal>(G));
-
-  for (size_t I = 0; I < Funcs.size(); ++I) {
-    const WasmFunction &Func = Funcs[I];
-    const WasmSignature &Sig = Types[FuncTypes[I]];
-    InputFunction *F = make<InputFunction>(Sig, &Func, this);
-    F->copyRelocations(*CodeSection);
-    Functions.emplace_back(F);
-  }
-
-  // Populate `Symbols` based on the WasmSymbols in the object
-  for (const SymbolRef &Sym : WasmObj->symbols()) {
-    const WasmSymbol &WasmSym = WasmObj->getWasmSymbol(Sym.getRawDataRefImpl());
-
-    if (Symbol *Sym = createDefined(WasmSym))
-      Symbols.push_back(Sym);
-    else
-      Symbols.push_back(createUndefined(WasmSym));
-  }
-}
-
 Symbol *ObjFile::createDefined(const WasmSymbol &Sym) {
   if (!Sym.isDefined())
     return nullptr;
 
   switch (Sym.Info.Kind) {
   case WASM_SYMBOL_TYPE_FUNCTION: {
-    InputFunction *Func = getFunction(Sym);
+    InputFunction *Func =
+        Functions[Sym.Info.ElementIndex - WasmObj->getNumImportedFunctions()];
     if (isExcludedByComdat(Func)) {
       Func->Live = false;
       return nullptr;
     }
 
     if (Sym.isBindingLocal())
       return make<DefinedFunction>(Sym.Info.Name, Sym.Info.Flags, this, Func);
     return Symtab->addDefinedFunction(Sym.Info.Name, Sym.Info.Flags, this,
                                       Func);
   }
   case WASM_SYMBOL_TYPE_DATA: {
-    InputSegment *Seg = getSegment(Sym);
+    InputSegment *Seg = Segments[Sym.Info.DataRef.Segment];
     if (isExcludedByComdat(Seg)) {
       Seg->Live = false;
       return nullptr;
     }
 
     uint32_t Offset = Sym.Info.DataRef.Offset;
     uint32_t Size = Sym.Info.DataRef.Size;
 
     if (Sym.isBindingLocal())
       return make<DefinedData>(Sym.Info.Name, Sym.Info.Flags, this, Seg, Offset,
                                Size);
     return Symtab->addDefinedData(Sym.Info.Name, Sym.Info.Flags, this, Seg,
                                   Offset, Size);
   }
   case WASM_SYMBOL_TYPE_GLOBAL:
+    InputGlobal *Global =
+        Globals[Sym.Info.ElementIndex - WasmObj->getNumImportedGlobals()];
     if (Sym.isBindingLocal())
-      return make<DefinedGlobal>(Sym.Info.Name, Sym.Info.Flags, this,
-                                 getGlobal(Sym));
+      return make<DefinedGlobal>(Sym.Info.Name, Sym.Info.Flags, this, Global);
     return Symtab->addDefinedGlobal(Sym.Info.Name, Sym.Info.Flags, this,
-                                    getGlobal(Sym));
+                                    Global);
   }
   llvm_unreachable("unkown symbol kind");
 }
 
 Symbol *ObjFile::createUndefined(const WasmSymbol &Sym) {
   StringRef Name = Sym.Info.Name;
   uint32_t Flags = Sym.Info.Flags;