[PATCH] D104945: [WebAssembly] Added initial type checker to MC Assembler

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a subscriber: pmatos.
sbc100 added a comment.

lgtm

We should probably wait for @dschuff input before landing.



================
Comment at: llvm/include/llvm/CodeGen/MachineModuleInfoImpls.h:113
+
+  StringSet<> MachineSymbolsUsed;
+};
----------------
The classes above seem to track sets of `MCSymbol*` (see SymbolListTy).  Would that make sense here too (rather than StringRef)?


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:248
+  std::map<unsigned, TypeStack> TypedLabels;
+  std::vector<wasm::ValType> LocalTypes;
+  std::vector<wasm::ValType> ReturnTypes;
----------------
Why not SmallVector like above?  


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:257
       : MCTargetAsmParser(Options, STI, MII), Parser(Parser),
         Lexer(Parser.getLexer()) {
     setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
----------------
Can we do `SkipTypeCheck(Options.MCNoTypeCheck)` here in the member initializer?   (same with `is64(STI.getTargetTriple().isArch64Bit())`?

Then the don't need to inline `= false` above either I guess?


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:269
+    if (BufferName == "<inline asm>")
+      SkipTypeCheck = true;
   }
----------------
Is this a permanent caveat or should we have a TODO here? 


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:1195
+    return false;
+  }
+
----------------
All this new code here makes me wonder if the typechecker should be its own class with its own state.   Maybe not worth it?


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:1361
+  assert(false && "mnemonic not found");
+  return StringRef();
+}
----------------
I surprised this functionality doesn't exist somewhere else already..


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:212
+MCSymbol *WebAssemblyAsmPrinter::SetWasmSymType(StringRef Name) {
+  auto *WasmSym = cast<MCSymbolWasm>(GetExternalSymbolSymbol(Name));
+  const WebAssemblySubtarget &Subtarget = getSubtarget();
----------------
Is this function called maybe times with the same name?   Can/shouldSetWasmSymType we avoid re-doing the work herein?

I just saw you have a FIXME for that already below.  Maybe we can tell if this symbols has already had its type set and early out?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:366
+  for (const auto &F : M) {
+    if (F.hasAddressTaken()) {
+      MCSymbolWasm *FunctionTable =
----------------
The old `HasAddressTakenFunction` was only set if `!F.isIntrinsic()` too.. is this change OK/indended?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:375
   for (const auto &G : M.globals()) {
+    auto S = G.getName();
     if (!G.hasInitializer() && G.hasExternalLinkage() &&
----------------
Left over debugging?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:528
+  AsmPrinter::emitLinkage(GV, Sym);
+  // This gets called before the function label and type are emitted.
+  // We use it to emit signatures of external functions.
----------------
So other AsmPrinters use this print the linkage of a single symbol, but we are co-opting to print the all signatures?   


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:531
+  // FIXME casts!
+  ((WebAssemblyAsmPrinter *)this)->emitExternalDecls(*MMI->getModule());
+}
----------------
Is this just const cast?  Why not use `const_cast`?  


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h:86
                                        bool &InvokeDetected);
+  MCSymbol *SetWasmSymType(StringRef Name);
+  void emitExternalDecls(const Module &M);
----------------
I think the direction is towards `lowerSnakeCase` in this API.   Also it would match better with `getMCSymbolForFunction` above to call this `setWasmSymType`.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:16
 multiclass TABLE<WebAssemblyRegClass rt> {
-  defm TABLE_GET_#rt : I<(outs rt:$res), (ins table32_op:$table),
+  defm TABLE_GET_#rt : I<(outs rt:$res), (ins table32_op:$table, I32:$i),
                          (outs), (ins table32_op:$table),
----------------
I don't really grok this part of the change.   @pmatos will know more.   Can we split part out since it feels like its own bugfix.   How was it not breaking stuff?  (Or was it?)


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:94
     const MachineOperand &MO) const {
-  const char *Name = MO.getSymbolName();
-  auto *WasmSym = cast<MCSymbolWasm>(Printer.GetExternalSymbolSymbol(Name));
-  const WebAssemblySubtarget &Subtarget = Printer.getSubtarget();
-
-  // Except for certain known symbols, all symbols used by CodeGen are
-  // functions. It's OK to hardcode knowledge of specific symbols here; this
-  // method is precisely there for fetching the signatures of known
-  // Clang-provided symbols.
-  if (strcmp(Name, "__stack_pointer") == 0 || strcmp(Name, "__tls_base") == 0 ||
-      strcmp(Name, "__memory_base") == 0 || strcmp(Name, "__table_base") == 0 ||
-      strcmp(Name, "__tls_size") == 0 || strcmp(Name, "__tls_align") == 0) {
-    bool Mutable =
-        strcmp(Name, "__stack_pointer") == 0 || strcmp(Name, "__tls_base") == 0;
-    WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
-    WasmSym->setGlobalType(wasm::WasmGlobalType{
-        uint8_t(Subtarget.hasAddr64() ? wasm::WASM_TYPE_I64
-                                      : wasm::WASM_TYPE_I32),
-        Mutable});
-    return WasmSym;
-  }
-
-  SmallVector<wasm::ValType, 4> Returns;
-  SmallVector<wasm::ValType, 4> Params;
-  if (strcmp(Name, "__cpp_exception") == 0) {
-    WasmSym->setType(wasm::WASM_SYMBOL_TYPE_EVENT);
-    // We can't confirm its signature index for now because there can be
-    // imported exceptions. Set it to be 0 for now.
-    WasmSym->setEventType(
-        {wasm::WASM_EVENT_ATTRIBUTE_EXCEPTION, /* SigIndex */ 0});
-    // We may have multiple C++ compilation units to be linked together, each of
-    // which defines the exception symbol. To resolve them, we declare them as
-    // weak.
-    WasmSym->setWeak(true);
-    WasmSym->setExternal(true);
-
-    // All C++ exceptions are assumed to have a single i32 (for wasm32) or i64
-    // (for wasm64) param type and void return type. The reaon is, all C++
-    // exception values are pointers, and to share the type section with
-    // functions, exceptions are assumed to have void return type.
-    Params.push_back(Subtarget.hasAddr64() ? wasm::ValType::I64
-                                           : wasm::ValType::I32);
-  } else { // Function symbols
-    WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
-    getLibcallSignature(Subtarget, Name, Returns, Params);
-  }
-  auto Signature =
-      std::make_unique<wasm::WasmSignature>(std::move(Returns), std::move(Params));
-  WasmSym->setSignature(Signature.get());
-  Printer.addSignature(std::move(Signature));
-
-  return WasmSym;
+  return Printer.SetWasmSymType(MO.getSymbolName());
 }
----------------
Actually maybe we should just call this method something like getOrCreateWasmSymbol?  (don't feel strongly about this).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104945/new/

https://reviews.llvm.org/D104945