[WebAssembly] Update WebAssemblyAsmTypeCheck for table.get

[nokotan]

Summary

This patch is aimed to resolve GitHub Issue #53789.

[sbc100]

Thanks for working on this!

lld/test/wasm/funcref.s

23 ↗

(On Diff #410214)

I think this test probably belongs in llvm/test/MC/WebAssembly/ since this change doesn't look like its related to the linker.

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp

200

Why is this needed? We don't support tables that contain i32 or i64 values do we?

[pmatos]

Thanks for the patch. In addition to what @sbc100 wrote, some further comments inline.

lld/test/wasm/funcref.s

23 ↗

(On Diff #410214)

+1 to the comment from @sbc100 and also maybe change the filename to something more descriptive. For example, tableget-funcref-typecheck.s.

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp

195

If we are doing a getTable here, the only valid case here is WASM_SYMBOL_TYPE_TABLE. I think the others can be removed and the switch simplified to an if.

[aardappel]

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp

185

This function is almost identically copy-pasted from getGlobal, please look into a way of merging these two functions cleanly.

[nokotan]

This diff has 2 changes.

• Moved test case 'funcref.s'
• Simplify getTable

[sbc100]

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp

195

Can you use an early return here to avoid the else?

if (if (WasmSym->getType().getValueOr(wasm::WASM_SYMBOL_TYPE_DATA) !=
      wasm::WASM_SYMBOL_TYPE_TABLE)
  return typeError(ErrorLoc, StringRef("symbol ") + WasmSym->getName() + ...

Type = static_cast<wasm::ValType>(WasmSym->getTableType().ElemType);
return false;

[nokotan]

Updated.

[aardappel]

I see my comment has not been addressed yet.

[nokotan]

Applied clang-format.

[pmatos]

Thanks for your time working on the patch. It would be great to address @aardappel 's comments. The idea is to merge the function getGlobal with getTable, since they are so similar. I suppose into something like getGlobalOrTable. Add another switch case for wasm::WASM_SYMBOL_TYPE_TABLE to return the appropriate type.

[nokotan]

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp

185

Yes, originally this function was cloned from getGlobal.
For now, I guess that getTable does not have to get merged with getGlobal, since getTable is already modified.

[sbc100]

In D120229#3349369, @aardappel wrote:

I see my comment has not been addressed yet.

I don't see see any nice way to merge these two functions. Since you made the original comment getTable has been simplified to make it more distinct. Perhaps there is some way to templatize, but I don't see it. I'd be OK with landing this change as is.

[nokotan]

I don't have access rights to the llvm repository, arc land failed from my terminal.

[sbc100]

In D120229#3357544, @nokotan wrote:

I don't have access rights to the llvm repository, arc land failed from my terminal.

@pmatos do you want to take care of landing this? (since its kind of your area). If you are busy can also take care of it.

[pmatos]

In D120229#3358146, @sbc100 wrote:

In D120229#3357544, @nokotan wrote:

I don't have access rights to the llvm repository, arc land failed from my terminal.

@pmatos do you want to take care of landing this? (since its kind of your area). If you are busy can also take care of it.

Sure - I will land it.

[DavidSpickett]

This caused two test failures on one of our bots: https://lab.llvm.org/buildbot/#/builders/183/builds/3684

For the new test, it's possible you had a wasm-ld on your path and that's why it worked locally.

Please investigate then reland.

[pmatos]

In D120229#3359775, @DavidSpickett wrote:

This caused two test failures on one of our bots: https://lab.llvm.org/buildbot/#/builders/183/builds/3684

For the new test, it's possible you had a wasm-ld on your path and that's why it worked locally.

Please investigate then reland.

Thanks David, yes that was the issue.

[pmatos]

@nokotan The test line should be something like: llvm-mc -mattr=+reference-types -triple=wasm32-unknown-unknown -filetype=obj -o - < %s | obj2yaml | FileCheck %s
However, I notice that MC/WebAssembly/tables.s is failing as well. Can you pls take a look?

[sbc100]

Looks like the llvm/test/MC/WebAssembly/funcref-from-table.s test was incorrectly relying on wasm-ld. It should only dependent of obj2yaml and/or llvm-objdump. Looks like the change was reverted for now.

[nokotan]

• Update test directive in funcref.s
• Add table.fill case in WebAssemblyAsmTypeCheck

[nokotan]

Rebasing branch

[pmatos]

Thanks - all tests pass, landing again.

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h

class WebAssemblyAsmTypeCheck final {
   bool typeError(SMLoc ErrorLoc, const Twine &Msg);
   bool popType(SMLoc ErrorLoc, Optional<wasm::ValType> EVT);
   bool getLocal(SMLoc ErrorLoc, const MCInst &Inst, wasm::ValType &Type);
   bool checkEnd(SMLoc ErrorLoc, bool PopVals = false);
   bool checkSig(SMLoc ErrorLoc, const wasm::WasmSignature &Sig);
   bool getSymRef(SMLoc ErrorLoc, const MCInst &Inst,
                  const MCSymbolRefExpr *&SymRef);
   bool getGlobal(SMLoc ErrorLoc, const MCInst &Inst, wasm::ValType &Type);
+  bool getTable(SMLoc ErrorLoc, const MCInst &Inst, wasm::ValType &Type);
 
 public:
   WebAssemblyAsmTypeCheck(MCAsmParser &Parser, const MCInstrInfo &MII, bool is64);
 
   void funcDecl(const wasm::WasmSignature &Sig);
   void localDecl(const SmallVector<wasm::ValType, 4> &Locals);
   void setLastSig(const wasm::WasmSignature &Sig) { LastSig = Sig; }
   bool endOfFunction(SMLoc ErrorLoc);

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp

case wasm::WASM_SYMBOL_TYPE_DATA:
     LLVM_FALLTHROUGH;
   default:
     return typeError(ErrorLoc, StringRef("symbol ") + WasmSym->getName() +
                                     " missing .globaltype");
   }
   return false;
 }
 
+bool WebAssemblyAsmTypeCheck::getTable(SMLoc ErrorLoc, const MCInst &Inst,
+                                       wasm::ValType &Type) {
+  const MCSymbolRefExpr *SymRef;
+  if (getSymRef(ErrorLoc, Inst, SymRef))
+    return true;
+  auto WasmSym = cast<MCSymbolWasm>(&SymRef->getSymbol());
+  if (WasmSym->getType().getValueOr(wasm::WASM_SYMBOL_TYPE_DATA) !=
+      wasm::WASM_SYMBOL_TYPE_TABLE)
+    return typeError(ErrorLoc, StringRef("symbol ") + WasmSym->getName() +
+                                   " missing .tabletype");
+  Type = static_cast<wasm::ValType>(WasmSym->getTableType().ElemType);
+  return false;
+}
+
 bool WebAssemblyAsmTypeCheck::endOfFunction(SMLoc ErrorLoc) {
   // Check the return types.
   for (auto RVT : llvm::reverse(ReturnTypes)) {
     if (popType(ErrorLoc, RVT))
       return true;
   }
   if (!Stack.empty()) {
     return typeError(ErrorLoc, std::to_string(Stack.size()) +
                                    " superfluous return values");
   }
if (Name == "local.get") {
     if (getGlobal(ErrorLoc, Inst, Type))
       return true;
     Stack.push_back(Type);
   } else if (Name == "global.set") {
     if (getGlobal(ErrorLoc, Inst, Type))
       return true;
     if (popType(ErrorLoc, Type))
       return true;
+  } else if (Name == "table.get") {
+    if (getTable(ErrorLoc, Inst, Type))
+      return true;
+    if (popType(ErrorLoc, wasm::ValType::I32))
+      return true;
+    Stack.push_back(Type);
+  } else if (Name == "table.set") {
+    if (getTable(ErrorLoc, Inst, Type))
+      return true;
+    if (popType(ErrorLoc, Type))
+      return true;
+    if (popType(ErrorLoc, wasm::ValType::I32))
+      return true;
+  } else if (Name == "table.fill") {
+    if (getTable(ErrorLoc, Inst, Type))
+      return true;
+    if (popType(ErrorLoc, wasm::ValType::I32))
+      return true;
+    if (popType(ErrorLoc, Type))
+      return true;
+    if (popType(ErrorLoc, wasm::ValType::I32))
+      return true;
   } else if (Name == "drop") {
     if (popType(ErrorLoc, {}))
       return true;
   } else if (Name == "end_block" || Name == "end_loop" || Name == "end_if" ||
              Name == "else" || Name == "end_try") {
     if (checkEnd(ErrorLoc, Name == "else"))
       return true;
     if (Name == "end_block")

llvm/test/MC/WebAssembly/funcref-from-table.s

+# RUN: llvm-mc -mattr=+reference-types -triple=wasm32-unknown-unknown -filetype=obj -o - < %s | obj2yaml | FileCheck %s
+
+.globl __indirect_function_table
+.tabletype __indirect_function_table, funcref
+
+.globl obtain_funcref_from_table_index
+
+obtain_funcref_from_table_index:
+  .functype obtain_funcref_from_table_index(i32) -> (funcref)
+  local.get 0
+  table.get __indirect_function_table
+  end_function
+
+#      CHECK:  Sections:
+# CHECK-NEXT:    - Type:            TYPE
+# CHECK-NEXT:      Signatures:
+# CHECK-NEXT:        - Index:           0
+# CHECK-NEXT:          ParamTypes:
+# CHECK-NEXT:            - I32
+# CHECK-NEXT:          ReturnTypes:
+# CHECK-NEXT:            - FUNCREF