[CodeGen][WebAssembly] Better lowering for WASM_SYMBOL_TYPE_GLOBAL symbols

[wingo]

Summary

As we have been missing support for WebAssembly globals on the IR level,
the lowering of WASM_SYMBOL_TYPE_GLOBAL to IR was incomplete. This
commit fleshes out the lowering support, lowering references to and
definitions of addrspace(1) values to correctly typed
WASM_SYMBOL_TYPE_GLOBAL symbols.

Depends on D101608.

[wingo]

Hi! Please take a look. The code duplication is horrible and suggestions to fix are most welcome. Normally, overriding emitGlobalVariable would be sufficient, but we also need to visit uses of Wasm globals, even if they aren't defined in the compilation unit. The code in GetGlobalAddressSymbol would be sufficient, but we also need to visit definitions that aren't used. Irritating!

[tlively]

@sbc100, you know a lot more about this symbol stuff than I do. Could you take a look at this?

[sbc100]

This looks great!

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

179

Why this? As it happens I think that globals are *all* thread local by default (at least we assume they are in the current threading ABI in llvm and emscripten). We don't have a way to share global between threads to (like we do with memory).

204

There is no actual initializer here.. it will always be zero I guess? Do we need a TODO?

218

Why not leave this code alone and omit the emitGlobalType above?

307

Similar to other PR.. I wish we could find a better work than Object in isObjectAddressSpace

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

68

Regarding the duplication.. I think we already suffer from a little of that with the existing code for handling functions (See WasmSym->setSignature below and in emitFunctionBodyStart and in emitEndOfAsmFile). So I guess the makes it ok ? :-/

[wingo]

Rename "object" to "managed", and address feedback

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

179

Good point; I hadn't thought about this. For context I was looking at what the base "getGlobalVariable" does and where it has conditional blocks for non-default flags, and asserting that no one had made non-default settings on the symbol.

But... semantically what's the story here? I guess that although you can share globals between instances, you can't post them to a web worker, so they are always thread-local.

Should WASM_SYMBOL_TYPE_GLOBAL symbols be marked by default as being thread-local then, perhaps?

204

Added the TODO. This one is tricky because there's no MC support for global initializers yet, AFAIU.

Not sure how it should work, tbh; though the binary format specifies that this should be a sequence of constant instructions followed by end, I think I would want to restrict this one from the MC side to be some form of MCConstantExpr, serialized either as part of the .globaltype line or as a new .globalvalue SYM, EXPR form. WDYT? Would be a followup I guess.

218

Because we need to record the type of the extern global somewhere in the .s file. Will add to comment. Sound OK to you?

307

Changed to "managed"; lmk what you think!

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

68

Fair enough :)

[sbc100]

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

179

Should WASM_SYMBOL_TYPE_GLOBAL symbols be marked by default as being thread-local then, perhaps?

I guess so .. at least if we are talking about the current threading support on the web. In the future globals will most likely be share-able.. or on other targets they might even be shared by default. But given that llvm's thread support is currently only used by the web targets it would make sense to me yes. Does't have to be part of this change though.

204

Not sure how it should work, tbh; though the binary format specifies that this should be a sequence of constant instructions followed by end, I think I would want to restrict this one from the MC side to be some form of MCConstantExpr, serialized either as part of the .globaltype line or as a new .globalvalue SYM, EXPR form. WDYT? Would be a followup I guess.

Yeah, I'm not sure about this either. Using actual instructions seems overkill, but at the same time matches the spec. I think it would be great if we could make it work... but also a more declarative mode might sense given the severe restrictions on instructions in cont expressions today.

How close are you to needing initializer values here? Can we punt for a little longer? :)

[wingo]

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

204

Thanks for review & sorry I forgot to reply :) Yeah can wait on the initializers -- in most cases I am interested in, the default value will be fine.

[wingo]

Rename "managed" address space to "wasm var".

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h

public:
 
   //===------------------------------------------------------------------===//
   // AsmPrinter Implementation.
   //===------------------------------------------------------------------===//
 
   void emitEndOfAsmFile(Module &M) override;
   void EmitProducerInfo(Module &M);
   void EmitTargetFeatures(Module &M);
+  void emitGlobalVariable(const GlobalVariable *GV) override;
   void emitJumpTableInfo() override;
   void emitConstantPool() override;
   void emitFunctionBodyStart() override;
   void emitInstruction(const MachineInstr *MI) override;
   bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                        const char *ExtraCode, raw_ostream &OS) override;
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
                              const char *ExtraCode, raw_ostream &OS) override;

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

if (EnableEmEH && isEmscriptenInvokeName(F->getName())) {
     WasmSym = cast<MCSymbolWasm>(
         GetExternalSymbolSymbol(getEmscriptenInvokeSymbolName(Sig)));
   } else {
     WasmSym = cast<MCSymbolWasm>(getSymbol(F));
   }
   return WasmSym;
 }
 
+void WebAssemblyAsmPrinter::emitGlobalVariable(const GlobalVariable *GV) {
+  if (!WebAssembly::isWasmVarAddressSpace(GV->getAddressSpace())) {
+    AsmPrinter::emitGlobalVariable(GV);
+    return;
+  }
+
+  assert(!GV->isThreadLocal());
+
+  MCSymbolWasm *Sym = cast<MCSymbolWasm>(getSymbol(GV));
+
+  if (!Sym->getType()) {
+    const WebAssemblyTargetLowering &TLI = *Subtarget->getTargetLowering();
+    SmallVector<EVT, 1> VTs;
+    ComputeValueVTs(TLI, GV->getParent()->getDataLayout(), GV->getValueType(),
+                    VTs);
+    if (VTs.size() != 1 ||
+        TLI.getNumRegisters(GV->getParent()->getContext(), VTs[0]) != 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});
+  }
+
+  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).
+    OutStreamer->AddBlankLine();
+  }
+}
+
 void WebAssemblyAsmPrinter::emitEndOfAsmFile(Module &M) {
   for (auto &It : OutContext.getSymbols()) {
     // Emit .globaltype, .eventtype, or .tabletype declarations.
     auto Sym = cast<MCSymbolWasm>(It.getValue());
-    if (Sym->getType() == wasm::WASM_SYMBOL_TYPE_GLOBAL)
+    if (Sym->getType() == wasm::WASM_SYMBOL_TYPE_GLOBAL) {
+      // .globaltype already handled by emitGlobalVariable for defined
+      // variables; here we make sure the types of external wasm globals get
+      // written to the file.
+      if (Sym->isUndefined())
         getTargetStreamer()->emitGlobalType(Sym);
-    else if (Sym->getType() == wasm::WASM_SYMBOL_TYPE_EVENT)
+    } else if (Sym->getType() == wasm::WASM_SYMBOL_TYPE_EVENT)
       getTargetStreamer()->emitEventType(Sym);
     else if (Sym->getType() == wasm::WASM_SYMBOL_TYPE_TABLE)
       getTargetStreamer()->emitTableType(Sym);
   }
 
   DenseSet<MCSymbol *> InvokeSymbols;
   bool HasAddressTakenFunction = false;
   for (const auto &F : M) {
void WebAssemblyAsmPrinter::emitEndOfAsmFile(Module &M) {
   // but for now manually mark the table as live.
   if (HasAddressTakenFunction) {
     MCSymbolWasm *FunctionTable =
         WebAssembly::getOrCreateFunctionTableSymbol(OutContext, Subtarget);
     OutStreamer->emitSymbolAttribute(FunctionTable, MCSA_NoDeadStrip);
   }
 
   for (const auto &G : M.globals()) {
-    if (!G.hasInitializer() && G.hasExternalLinkage()) {
-      if (G.getValueType()->isSized()) {
-        uint16_t Size = M.getDataLayout().getTypeAllocSize(G.getValueType());
-        OutStreamer->emitELFSize(getSymbol(&G),
-                                 MCConstantExpr::create(Size, OutContext));
-      }
-    }
-  }
+    if (!G.hasInitializer() && G.hasExternalLinkage() &&
+        !WebAssembly::isWasmVarAddressSpace(G.getAddressSpace()) &&
+        G.getValueType()->isSized()) {
 
   if (const NamedMDNode *Named = M.getNamedMetadata("wasm.custom_sections")) {
     for (const Metadata *MD : Named->operands()) {
       const auto *Tuple = dyn_cast<MDTuple>(MD);
       if (!Tuple || Tuple->getNumOperands() != 2)
         continue;
       const MDString *Name = dyn_cast<MDString>(Tuple->getOperand(0));
       const MDString *Contents = dyn_cast<MDString>(Tuple->getOperand(1));

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

 /// This file contains code to lower WebAssembly MachineInstrs to their
 /// corresponding MCInst records.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "WebAssemblyMCInstLower.h"
 #include "TargetInfo/WebAssemblyTargetInfo.h"
 #include "Utils/WebAssemblyTypeUtilities.h"
+#include "Utils/WebAssemblyUtilities.h"
 #include "WebAssemblyAsmPrinter.h"
 #include "WebAssemblyMachineFunctionInfo.h"
 #include "WebAssemblyRuntimeLibcallSignatures.h"
 #include "llvm/CodeGen/AsmPrinter.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCContext.h"

 extern cl::opt<bool> EnableEmException;
 extern cl::opt<bool> EnableEmSjLj;
 
 static void removeRegisterOperands(const MachineInstr *MI, MCInst &OutMI);
 
 MCSymbol *
 WebAssemblyMCInstLower::GetGlobalAddressSymbol(const MachineOperand &MO) const {
   const GlobalValue *Global = MO.getGlobal();
-  if (!isa<Function>(Global))
-    return cast<MCSymbolWasm>(Printer.getSymbol(Global));
+  if (!isa<Function>(Global)) {
+    auto *WasmSym = cast<MCSymbolWasm>(Printer.getSymbol(Global));
+    // If the symbol doesn't have an explicit WasmSymbolType yet and the
+    // GlobalValue is actually a WebAssembly global, then ensure the symbol is a
+    // WASM_SYMBOL_TYPE_GLOBAL.
+    if (WebAssembly::isWasmVarAddressSpace(Global->getAddressSpace()) &&
+        !WasmSym->getType()) {
+      const MachineFunction &MF = *MO.getParent()->getParent()->getParent();
+      const TargetMachine &TM = MF.getTarget();
+      const Function &CurrentFunc = MF.getFunction();
+      SmallVector<MVT, 1> VTs;
+      computeLegalValueVTs(CurrentFunc, TM, Global->getValueType(), VTs);
+      if (VTs.size() != 1)
+        report_fatal_error("Aggregate globals not yet implemented");
+
+      bool Mutable = true;
+      wasm::ValType Type = WebAssembly::toValType(VTs[0]);
+      WasmSym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
+      WasmSym->setGlobalType(wasm::WasmGlobalType{uint8_t(Type), Mutable});
+    }
+    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();
 
   SmallVector<MVT, 1> ResultMVTs;
   SmallVector<MVT, 4> ParamMVTs;

llvm/test/CodeGen/WebAssembly/global-get.ll

 ; RUN: llc < %s --mtriple=wasm32-unknown-unknown -asm-verbose=false | FileCheck %s
 
 @i32_global = local_unnamed_addr addrspace(1) global i32 undef
 @i64_global = local_unnamed_addr addrspace(1) global i64 undef
 @f32_global = local_unnamed_addr addrspace(1) global float undef
 @f64_global = local_unnamed_addr addrspace(1) global double undef
 
+@i32_external_used = external addrspace(1) global i32
+@i32_external_unused = external addrspace(1) global i32
+
 define i32 @return_i32_global() {
 ; CHECK-LABEL: return_i32_global:
 ; CHECK-NEXT: functype       return_i32_global () -> (i32)
 ; CHECK-NEXT: global.get i32_global
 ; CHECK-NEXT: end_function
   %v = load i32, i32 addrspace(1)* @i32_global
   ret i32 %v
 }

 ; CHECK-LABEL: return_f64_global:
 ; CHECK-NEXT: functype       return_f64_global () -> (f64)
 ; CHECK-NEXT: global.get f64_global
 ; CHECK-NEXT: end_function
   %v = load double, double addrspace(1)* @f64_global
   ret double %v
 }
 
+define i32 @return_extern_i32_global() {
+; CHECK-LABEL: return_extern_i32_global:
+; CHECK-NEXT: functype       return_extern_i32_global () -> (i32)
+; CHECK-NEXT: global.get i32_external_used
+; CHECK-NEXT: end_function
+  %v = load i32, i32 addrspace(1)* @i32_external_used
+  ret i32 %v
+}
+
 
-;; LLVM doesn't yet declare proper WebAssembly globals for these values,
-;; instead placing them in linear memory.  To fix in a followup.
-; FIXME-CHECK: .globl i32_global
-; FIXME-CHECK: .globaltype i32_global, i32
-; FIXME-CHECK: .globl i64_global
-; FIXME-CHECK: .globaltype i64_global, i64
-; FIXME-CHECK: .globl f32_global
-; FIXME-CHECK: .globaltype f32_global, f32
-; FIXME-CHECK: .globl f64_global
-; FIXME-CHECK: .globaltype f64_global, f64
+; CHECK: .globl i32_global
+; CHECK: .globaltype i32_global, i32
+; CHECK-LABEL: i32_global:
+
+; CHECK: .globl i64_global
+; CHECK: .globaltype i64_global, i64
+; CHECK-LABEL: i64_global:
+
+; CHECK: .globl f32_global
+; CHECK: .globaltype f32_global, f32
+; CHECK-LABEL: f32_global:
+
+; CHECK: .globl f64_global
+; CHECK: .globaltype f64_global, f64
+; CHECK-LABEL: f64_global:
+
+; CHECK-NOT: .global i32_external_used
+; CHECK: .globaltype i32_external_used, i32
+; CHECK-NOT: i32_external_used:
+
+; CHECK-NOT: .global i32_external_unused
+; CHECK: .globaltype i32_external_unused, i32
+; CHECK-NOT: i32_external_unused:

llvm/test/CodeGen/WebAssembly/global-set.ll

 ; CHECK-NEXT: functype       set_f64_global (f64) -> ()
 ; CHECK-NEXT: local.get 0
 ; CHECK-NEXT: global.set f64_global
 ; CHECK-NEXT: end_function
   store double %v, double addrspace(1)* @f64_global
   ret void
 }
 
-;; LLVM doesn't yet declare proper WebAssembly globals for these values,
-;; instead placing them in linear memory.  To fix in a followup.
-; FIXME-CHECK: .globl i32_global
-; FIXME-CHECK: .globaltype i32_global, i32
-; FIXME-CHECK: .globl i64_global
-; FIXME-CHECK: .globaltype i64_global, i64
-; FIXME-CHECK: .globl f32_global
-; FIXME-CHECK: .globaltype f32_global, f32
-; FIXME-CHECK: .globl f64_global
-; FIXME-CHECK: .globaltype f64_global, f64
+; CHECK: .globl i32_global
+; CHECK: .globaltype i32_global, i32
+; CHECK-LABEL: i32_global:
+
+; CHECK: .globl i64_global
+; CHECK: .globaltype i64_global, i64
+; CHECK-LABEL: i64_global:
+
+; CHECK: .globl f32_global
+; CHECK: .globaltype f32_global, f32
+; CHECK-LABEL: f32_global:
+
+; CHECK: .globl f64_global
+; CHECK: .globaltype f64_global, f64
+; CHECK-LABEL: f64_global: