[WebAssembly] Lower global syms representing tables with .tabletype

[pmatos]

Summary

This patch implements a fix to recognize global symbols that represent
WebAssembly appropriately and generate the necessary .tabletype
directives.

[pmatos]

@sbc100 @tlively This is still not working properly... but I would like to understand if this is the correct way to go about it.

[sbc100]

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h

118 ↗

(On Diff #393451)

I'm not sure this enumeration is best place to declare that type of a symbols. These other values all refer to how the symbol is used, but what type the symbol is. For example that distinction between function and data symbols does not appear here.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

1540 ↗

(On Diff #393451)

Why MO_TABLE_BASE_REL here?

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

172

If you do want to set the symbol type at this point why do it inside this if (MO.getOffset() != 0) { condition?

However, it seems like for other symbol types the type is already previously set elsewhere so this maybe is not the place to do it?

Looking at all the places where wasm symbols get their types set it looks like a rather mixed bag:

$ git grep 'setType(wasm::'
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:        SPSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
llvm/lib/MC/MCContext.cpp:  cast<MCSymbolWasm>(Begin)->setType(wasm::WASM_SYMBOL_TYPE_SECTION);
llvm/lib/MC/MCParser/WasmAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
llvm/lib/MC/MCParser/WasmAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
llvm/lib/MC/MCParser/WasmAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_DATA);
llvm/lib/MC/MCWasmStreamer.cpp:    Symbol->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_TABLE);
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_TAG);
llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:        WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp:    Sym->setType(wasm::WASM_SYMBOL_TYPE_TABLE);
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:    Sym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:    WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:    WasmSym->setType(wasm::WASM_SYMBOL_TYPE_DATA);
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:    WasmSym->setType(wasm::WASM_SYMBOL_TYPE_TAG);
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:    WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:      Sym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:  WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:  WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:  WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);

Do you have any information to do this t WebAssemblyMCInstLower::GetGlobalAddressSymbol?

[pmatos]

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

1540 ↗

(On Diff #393451)

That's a mistake. It should be MO_SYM_IS_..._TABLE.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

172

Yeah - I got the same feeling. Rather confusing about the best place to set this information. I will take a look at WebAssemblyMCInstLower::GetGlobalAddressSymbol. Thanks for the suggestion.

[pmatos]

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h

118 ↗

(On Diff #393451)

This is the place to set TargetFlags, and we cannot set the type of the symbol directly here because we don't have access to the MCSymbol, so as far as I can understand we need to set a TargetFlag and later on set the right MCSymbol type based on the address TargetFlags. Do you have a suggestion on where to set this information if not as a TargetFlag?

[sbc100]

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h

118 ↗

(On Diff #393451)

I'm afraid its been a while since I touched this code.. so I'm not sure I have any useful suggestions other than looking at existing code and practices.

[pmatos]

Fixes the issues found with previous patch.

We needed to ensure that GlobalVariable emissions supports table type
symbols as well as proper lowering.

@sbc100, I didn't ignore your comment regarding MO_SYM_IS_..._TABLE but
at this point I really don't know where to place this information for the
MC layer to act on it.

[pmatos]

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h

118 ↗

(On Diff #393451)

Thanks, I have been looking and didn't find a better place to tag a symbol with a property so that the MC layer can 'fix' the symbol type. I will keep looking, if you think this is an improper place for the symbol type tag.

[pmatos]

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

172

Me: Who was the fool who set the type of a table GetGlobalAddressSymbol to WASM_SYMBOL_TYPE_GLOBAL?

git blame: That was you...

Me: Doh!

[pmatos]

Patch simplification.

@sbc100 Thanks for the pointer to GetGlobalAddressSymbol (even though I should have
recalled my own change). I hope this patch is easier to accept.

[pmatos]

Making the linter happy

[sbc100]

Oh nice! This looks way better now. Glad you figured it out, I'm not sure if I would have been much help!

[pmatos]

In D115511#3189206, @sbc100 wrote:

Oh nice! This looks way better now. Glad you figured it out, I'm not sure if I would have been much help!

Thanks for the review - happy that we manage to find a minimal way to fix this.

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

if (VTs.size() != 1 ||
       report_fatal_error("Aggregate globals not yet implemented");
     MVT VT = TLI.getRegisterType(GV->getParent()->getContext(), VTs[0]);
     bool Mutable = true;
     wasm::ValType Type = WebAssembly::toValType(VT);
     Sym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
     Sym->setGlobalType(wasm::WasmGlobalType{uint8_t(Type), Mutable});
   }
 
+  // If the GlobalVariable refers to a table, we handle it here instead of
+  // in emitExternalDecls
+  if (Sym->isTable()) {
+    getTargetStreamer()->emitTableType(Sym);
+    return;
+  }
+
   emitVisibility(Sym, GV->getVisibility(), !GV->isDeclaration());
   if (GV->hasInitializer()) {
     assert(getSymbolPreferLocal(*GV) == Sym);
     emitLinkage(GV, Sym);
     getTargetStreamer()->emitGlobalType(Sym);
     OutStreamer->emitLabel(Sym);
     // TODO: Actually emit the initializer value.  Otherwise the global has the
     // default value for its type (0, ref.null, etc).

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

if (WebAssembly::isWasmVarAddressSpace(Global->getAddressSpace()) &&
       Type *GlobalVT = Global->getValueType();
       SmallVector<MVT, 1> VTs;
       computeLegalValueVTs(CurrentFunc, TM, GlobalVT, VTs);
 
       // Tables are represented as Arrays in LLVM IR therefore
       // they reach this point as aggregate Array types with an element type
       // that is a reference type.
       wasm::ValType Type;
+      bool IsTable = false;
       if (GlobalVT->isArrayTy() &&
           WebAssembly::isRefType(GlobalVT->getArrayElementType())) {
         MVT VT;
+        IsTable = true;
         switch (GlobalVT->getArrayElementType()->getPointerAddressSpace()) {
         case WebAssembly::WasmAddressSpace::WASM_ADDRESS_SPACE_FUNCREF:
           VT = MVT::funcref;
           break;
         case WebAssembly::WasmAddressSpace::WASM_ADDRESS_SPACE_EXTERNREF:
           VT = MVT::externref;
           break;
         default:
           report_fatal_error("unhandled address space type");
         }
         Type = WebAssembly::toValType(VT);
       } else if (VTs.size() == 1) {
         Type = WebAssembly::toValType(VTs[0]);
       } else
         report_fatal_error("Aggregate globals not yet implemented");
 
+      if (IsTable) {
+        WasmSym->setType(wasm::WASM_SYMBOL_TYPE_TABLE);
+        WasmSym->setTableType(Type);
+      } else {
         WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
         WasmSym->setGlobalType(
             wasm::WasmGlobalType{uint8_t(Type), /*Mutable=*/true});
       }
+    }
     return WasmSym;
   }
 
   const auto *FuncTy = cast<FunctionType>(Global->getValueType());
   const MachineFunction &MF = *MO.getParent()->getParent()->getParent();
   const TargetMachine &TM = MF.getTarget();
   const Function &CurrentFunc = MF.getFunction();
 
if (MO.getOffset() != 0) {
     if (WasmSym->isTag())
       report_fatal_error("Tag indexes with offsets not supported");
     if (WasmSym->isTable())
       report_fatal_error("Table indexes with offsets not supported");
 
     Expr = MCBinaryExpr::createAdd(
         Expr, MCConstantExpr::create(MO.getOffset(), Ctx), Ctx);
   }
 
   return MCOperand::createExpr(Expr);
 }
 
 MCOperand WebAssemblyMCInstLower::lowerTypeIndexOperand(
     SmallVector<wasm::ValType, 1> &&Returns,
     SmallVector<wasm::ValType, 4> &&Params) const {
   auto Signature = std::make_unique<wasm::WasmSignature>(std::move(Returns),
                                                          std::move(Params));

llvm/test/CodeGen/WebAssembly/externref-tableget.ll

 ; CHECK-NEXT:  end_function
   %j = call i32 @get_offset()
   %off = add nsw i32 %i, %j
   %p = getelementptr [0 x %externref], [0 x %externref] addrspace (1)* @externref_table, i32 0, i32 %off
   %ref = load %externref, %externref addrspace(1)* %p
   ret %externref %ref
 }
 
-; CHECK: .globl externref_table
+; CHECK: .tabletype externref_table, externref

llvm/test/CodeGen/WebAssembly/externref-tableset.ll

 ; CHECK-NEXT:  end_function
   %j = call i32 @set_offset()
   %off = add nsw i32 %i, %j
   %p = getelementptr [0 x %externref], [0 x %externref] addrspace (1)* @externref_table, i32 0, i32 %off
   store %externref %g, %externref addrspace(1)* %p
   ret void
 }
 
-; CHECK: .globl externref_table
+; CHECK: .tabletype externref_table, externref

llvm/test/CodeGen/WebAssembly/funcref-table_call.ll

 ; RUN: llc < %s --mtriple=wasm32-unknown-unknown -asm-verbose=false -mattr=+reference-types | FileCheck %s
 
 %funcptr = type void () addrspace(20)*
 %funcref = type i8 addrspace(20)* ;; addrspace 20 is nonintegral
 
 @funcref_table = local_unnamed_addr addrspace(1) global [0 x %funcref] undef
 
 define void @call_funcref_from_table(i32 %i) {
   %p = getelementptr [0 x %funcref], [0 x %funcref] addrspace (1)* @funcref_table, i32 0, i32 %i
   %ref = load %funcref, %funcref addrspace(1)* %p
   %fn = bitcast %funcref %ref to %funcptr
   call addrspace(20) void %fn()
   ret void
 }
 
-; CHECK: .tabletype __funcref_call_table, funcref, 1
+; CHECK: .tabletype      __funcref_call_table, funcref, 1
 
 ; CHECK-LABEL: call_funcref_from_table:
 ; CHECK-NEXT: .functype       call_funcref_from_table (i32) -> ()
 ; CHECK-NEXT: i32.const       0
 ; CHECK-NEXT: local.get       0
 ; CHECK-NEXT: table.get       funcref_table
 ; CHECK-NEXT: table.set       __funcref_call_table
 ; CHECK-NEXT: i32.const       0
 ; CHECK-NEXT: call_indirect    __funcref_call_table, () -> ()
 ; CHECK-NEXT: i32.const       0
 ; CHECK-NEXT: ref.null_func
 ; CHECK-NEXT: table.set       __funcref_call_table
 ; CHECK-NEXT: end_function
 
+; CHECK: .tabletype     funcref_table, funcref
 
-; CHECK:     .globl funcref_table
-; CHECK-NEXT .globaltype     funcref_table, funcref

llvm/test/CodeGen/WebAssembly/funcref-tableget.ll

 ; CHECK-NEXT:  end_function
   %j = call i32 @get_offset()
   %off = add nsw i32 %i, %j
   %p = getelementptr [0 x %funcref], [0 x %funcref] addrspace (1)* @funcref_table, i32 0, i32 %off
   %ref = load %funcref, %funcref addrspace(1)* %p
   ret %funcref %ref
 }
 
-; CHECK: .globl funcref_table
+; CHECK: .tabletype funcref_table, funcref

llvm/test/CodeGen/WebAssembly/funcref-tableset.ll

 ; CHECK-NEXT:  end_function
   %j = call i32 @set_offset()
   %off = add nsw i32 %i, %j
   %p = getelementptr [0 x %funcref], [0 x %funcref] addrspace (1)* @funcref_table, i32 0, i32 %off
   store %funcref %g, %funcref addrspace(1)* %p
   ret void
 }
 
-; CHECK: .globl funcref_table
+; CHECK: .tabletype funcref_table, funcref