[WebAssembly][NFC] Refactor WasmSymbol type setting code

[pmatos]

Summary

This refactors some code dealing with setting Wasm symbol types.
Some of the code dealing with types was moved from
WebAssemblyUtilities to WebAssemblyTypeUtilities.

[pmatos]

@sbc100 Address review comments from D115749.

Make WasmSymbolSetType lower case to match what other functions in
WebAssemblyTypeUtilities do.

Also, this function is shared between MCInstLower and AsmPrinter,
therefore it needs to be in a common Utilities file. Since this
deals with types, it makes sense for that file to be
WebAssemblyTypeUtilities.

[pmatos]

@sbc100 I have now addressed all your comments, I feel like it is ready for review.

[sbc100]

Is this change completely non-functional? If so can you add NFC to the title?

llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp

20

Are both WebAssemblyUtilities.h and WebAssemblyTypeUtilities.h needed here?

[pmatos]

llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp

20

Yes, WebAssemblyUtilities.h is still needed for getOrCreateFunctionTableSymbol.

[pmatos]

Added [NFC] to the title, rebased on main and rechecked that tests are fine.

@sbc100 yes, this is NFC. Happy for this to land?

[sbc100]

LGTM % comment about emitTableType

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

201

Does removing this block not make this a functional change? Are there no tests that depend on emitTableType here?

[pmatos]

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

201

Argh, I missed this. Actually this should be in D118122, in any case apparently we have no test which tests this execution path. I will commit with these lines and change D118122 to remove it instead, so this is truly a NFC. Well done noticing this detail.

[pmatos]

Remove non-NFC change.

llvm/lib/Target/WebAssembly/Utils/WebAssemblyTypeUtilities.h

 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_WEBASSEMBLY_UTILS_WEBASSEMBLYTYPEUTILITIES_H
 #define LLVM_LIB_TARGET_WEBASSEMBLY_UTILS_WEBASSEMBLYTYPEUTILITIES_H
 
 #include "llvm/ADT/Optional.h"
 #include "llvm/BinaryFormat/Wasm.h"
+#include "llvm/IR/DerivedTypes.h"
+#include "llvm/MC/MCSymbolWasm.h"
 #include "llvm/Support/MachineValueType.h"
 
 namespace llvm {
 namespace WebAssembly {
 
 /// Used as immediate MachineOperands for block signatures
 enum class BlockType : unsigned {
   Invalid = 0x00,
   Void = 0x40,
   I32 = unsigned(wasm::ValType::I32),
   I64 = unsigned(wasm::ValType::I64),
   F32 = unsigned(wasm::ValType::F32),
   F64 = unsigned(wasm::ValType::F64),
   V128 = unsigned(wasm::ValType::V128),
   Externref = unsigned(wasm::ValType::EXTERNREF),
   Funcref = unsigned(wasm::ValType::FUNCREF),
   // Multivalue blocks (and other non-void blocks) are only emitted when the
   // blocks will never be exited and are at the ends of functions (see
   // WebAssemblyCFGStackify::fixEndsAtEndOfFunction). They also are never made
   // to pop values off the stack, so the exact multivalue signature can always
   // be inferred from the return type of the parent function in MCInstLower.
   Multivalue = 0xffff,
 };
 
+enum WasmAddressSpace : unsigned {
+  // Default address space, for pointers to linear memory (stack, heap, data).
+  WASM_ADDRESS_SPACE_DEFAULT = 0,
+  // A non-integral address space for pointers to named objects outside of
+  // linear memory: WebAssembly globals or WebAssembly locals.  Loads and stores
+  // to these pointers are lowered to global.get / global.set or local.get /
+  // local.set, as appropriate.
+  WASM_ADDRESS_SPACE_VAR = 1,
+  // A non-integral address space for externref values
+  WASM_ADDRESS_SPACE_EXTERNREF = 10,
+  // A non-integral address space for funcref values
+  WASM_ADDRESS_SPACE_FUNCREF = 20,
+};
+
+inline bool isDefaultAddressSpace(unsigned AS) {
+  return AS == WASM_ADDRESS_SPACE_DEFAULT;
+}
+inline bool isWasmVarAddressSpace(unsigned AS) {
+  return AS == WASM_ADDRESS_SPACE_VAR;
+}
+inline bool isValidAddressSpace(unsigned AS) {
+  return isDefaultAddressSpace(AS) || isWasmVarAddressSpace(AS);
+}
+inline bool isFuncrefType(const Type *Ty) {
+  return isa<PointerType>(Ty) &&
+         Ty->getPointerAddressSpace() ==
+             WasmAddressSpace::WASM_ADDRESS_SPACE_FUNCREF;
+}
+inline bool isExternrefType(const Type *Ty) {
+  return isa<PointerType>(Ty) &&
+         Ty->getPointerAddressSpace() ==
+             WasmAddressSpace::WASM_ADDRESS_SPACE_EXTERNREF;
+}
+inline bool isRefType(const Type *Ty) {
+  return isFuncrefType(Ty) || isExternrefType(Ty);
+}
+
 // Convert StringRef to ValType / HealType / BlockType
 
 Optional<wasm::ValType> parseType(StringRef Type);
 BlockType parseBlockType(StringRef Type);
 MVT parseMVT(StringRef Type);
 
 // Convert ValType or a list/signature of ValTypes to a string.
 

 std::string signatureToString(const wasm::WasmSignature *Sig);
 
 // Convert a MVT into its corresponding wasm ValType.
 wasm::ValType toValType(MVT Type);
 
 // Convert a register class to a wasm ValType.
 wasm::ValType regClassToValType(unsigned RC);
 
+/// Sets a Wasm Symbol Type.
+void wasmSymbolSetType(MCSymbolWasm *Sym, const Type *GlobalVT,
+                       const SmallVector<MVT, 1> &VTs);
+
 } // end namespace WebAssembly
 } // end namespace llvm
 
 #endif

llvm/lib/Target/WebAssembly/Utils/WebAssemblyTypeUtilities.cpp

wasm::ValType WebAssembly::regClassToValType(unsigned RC) {
   case WebAssembly::FUNCREFRegClassID:
     return wasm::ValType::FUNCREF;
   case WebAssembly::EXTERNREFRegClassID:
     return wasm::ValType::EXTERNREF;
   default:
     llvm_unreachable("unexpected type");
   }
 }
+
+void WebAssembly::wasmSymbolSetType(MCSymbolWasm *Sym, const Type *GlobalVT,
+                                    const SmallVector<MVT, 1> &VTs) {
+  assert(!Sym->getType());
+
+  // 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) {
+    Sym->setType(wasm::WASM_SYMBOL_TYPE_TABLE);
+    Sym->setTableType(Type);
+  } else {
+    Sym->setType(wasm::WASM_SYMBOL_TYPE_GLOBAL);
+    Sym->setGlobalType(wasm::WasmGlobalType{uint8_t(Type), /*Mutable=*/true});
+  }
+}

llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h

 /// This file contains the declaration of the WebAssembly-specific
 /// utility functions.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_WEBASSEMBLY_UTILS_WEBASSEMBLYUTILITIES_H
 #define LLVM_LIB_TARGET_WEBASSEMBLY_UTILS_WEBASSEMBLYUTILITIES_H
 
-#include "llvm/IR/DerivedTypes.h"
 #include "llvm/Support/CommandLine.h"
 
 namespace llvm {
 
 class MachineBasicBlock;
 class MachineInstr;
 class MachineOperand;
 class MCContext;
 class MCSymbolWasm;
 class WebAssemblyFunctionInfo;
 class WebAssemblySubtarget;
 
 namespace WebAssembly {
 
-enum WasmAddressSpace : unsigned {
-  // Default address space, for pointers to linear memory (stack, heap, data).
-  WASM_ADDRESS_SPACE_DEFAULT = 0,
-  // A non-integral address space for pointers to named objects outside of
-  // linear memory: WebAssembly globals or WebAssembly locals.  Loads and stores
-  // to these pointers are lowered to global.get / global.set or local.get /
-  // local.set, as appropriate.
-  WASM_ADDRESS_SPACE_VAR = 1,
-  // A non-integral address space for externref values
-  WASM_ADDRESS_SPACE_EXTERNREF = 10,
-  // A non-integral address space for funcref values
-  WASM_ADDRESS_SPACE_FUNCREF = 20,
-};
-
-inline bool isDefaultAddressSpace(unsigned AS) {
-  return AS == WASM_ADDRESS_SPACE_DEFAULT;
-}
-inline bool isWasmVarAddressSpace(unsigned AS) {
-  return AS == WASM_ADDRESS_SPACE_VAR;
-}
-inline bool isValidAddressSpace(unsigned AS) {
-  return isDefaultAddressSpace(AS) || isWasmVarAddressSpace(AS);
-}
-inline bool isFuncrefType(const Type *Ty) {
-  return isa<PointerType>(Ty) &&
-         Ty->getPointerAddressSpace() ==
-             WasmAddressSpace::WASM_ADDRESS_SPACE_FUNCREF;
-}
-inline bool isExternrefType(const Type *Ty) {
-  return isa<PointerType>(Ty) &&
-         Ty->getPointerAddressSpace() ==
-             WasmAddressSpace::WASM_ADDRESS_SPACE_EXTERNREF;
-}
-inline bool isRefType(const Type *Ty) {
-  return isFuncrefType(Ty) || isExternrefType(Ty);
-}
-
 bool isChild(const MachineInstr &MI, const WebAssemblyFunctionInfo &MFI);
 bool mayThrow(const MachineInstr &MI);
 
 // Exception handling / setjmp-longjmp handling command-line options
 extern cl::opt<bool> WasmEnableEmEH;   // asm.js-style EH
 extern cl::opt<bool> WasmEnableEmSjLj; // asm.js-style SjLJ
 extern cl::opt<bool> WasmEnableEH;     // EH using Wasm EH instructions
 extern cl::opt<bool> WasmEnableSjLj;   // SjLj using Wasm EH instructions

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

void WebAssemblyAsmPrinter::emitGlobalVariable(const GlobalVariable *GV) {
   }
 
   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});
+    SmallVector<MVT, 1> VTs;
+    Type *GlobalVT = GV->getValueType();
+    computeLegalValueVTs(TLI, GV->getParent()->getContext(),
+                         GV->getParent()->getDataLayout(), GlobalVT, VTs);
+    WebAssembly::wasmSymbolSetType(Sym, GlobalVT, VTs);
   }
 
   // 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);

llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp

 /// class. Some of the target-specific code is generated by tablegen in the file
 /// WebAssemblyGenFastISel.inc, which is #included here.
 ///
 /// TODO: kill flags
 ///
 //===----------------------------------------------------------------------===//
 
 #include "MCTargetDesc/WebAssemblyMCTargetDesc.h"
+#include "Utils/WebAssemblyTypeUtilities.h"
 #include "Utils/WebAssemblyUtilities.h"
 #include "WebAssembly.h"
 #include "WebAssemblyMachineFunctionInfo.h"
 #include "WebAssemblySubtarget.h"
 #include "WebAssemblyTargetMachine.h"
 #include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/CodeGen/FastISel.h"
 #include "llvm/CodeGen/FunctionLoweringInfo.h"
 #include "llvm/CodeGen/MachineConstantPool.h"

llvm/lib/Target/WebAssembly/WebAssemblyFrameLowering.cpp

 /// callee-saved registers to save, and no spill slots.
 ///
 /// The stack grows downward.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "WebAssemblyFrameLowering.h"
 #include "MCTargetDesc/WebAssemblyMCTargetDesc.h"
-#include "Utils/WebAssemblyUtilities.h"
+#include "Utils/WebAssemblyTypeUtilities.h"
 #include "WebAssembly.h"
 #include "WebAssemblyInstrInfo.h"
 #include "WebAssemblyMachineFunctionInfo.h"
 #include "WebAssemblySubtarget.h"
 #include "WebAssemblyTargetMachine.h"
 #include "llvm/CodeGen/Analysis.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineFunction.h"

llvm/lib/Target/WebAssembly/WebAssemblyLowerRefTypesIntPtrConv.cpp

 //=== WebAssemblyLowerRefTypesIntPtrConv.cpp -
 //                     Lower IntToPtr and PtrToInt on Reference Types   ---===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// Lowers IntToPtr and PtrToInt instructions on reference types to
 /// Trap instructions since they have been allowed to operate
 /// on non-integral pointers.
 ///
 //===----------------------------------------------------------------------===//
 
-#include "Utils/WebAssemblyUtilities.h"
+#include "Utils/WebAssemblyTypeUtilities.h"
 #include "WebAssembly.h"
 #include "WebAssemblySubtarget.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/Pass.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "wasm-lower-reftypes-intptr-conv"

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

if (WebAssembly::isWasmVarAddressSpace(Global->getAddressSpace()) &&
         !WasmSym->getType()) {
       const MachineFunction &MF = *MO.getParent()->getParent()->getParent();
       const TargetMachine &TM = MF.getTarget();
       const Function &CurrentFunc = MF.getFunction();
       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});
-      }
+      WebAssembly::wasmSymbolSetType(WasmSym, GlobalVT, VTs);
     }
     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();

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.h

public:
 
   bool isCFGStackified() const { return CFGStackified; }
   void setCFGStackified(bool Value = true) { CFGStackified = Value; }
 
   WasmEHFuncInfo *getWasmEHFuncInfo() const { return WasmEHInfo; }
   void setWasmEHFuncInfo(WasmEHFuncInfo *Info) { WasmEHInfo = Info; }
 };
 
+void computeLegalValueVTs(const WebAssemblyTargetLowering &TLI,
+                          LLVMContext &Ctx, const DataLayout &DL, Type *Ty,
+                          SmallVectorImpl<MVT> &ValueVTs);
+
 void computeLegalValueVTs(const Function &F, const TargetMachine &TM, Type *Ty,
                           SmallVectorImpl<MVT> &ValueVTs);
 
 // Compute the signature for a given FunctionType (Ty). Note that it's not the
 // signature for ContextFunc (ContextFunc is just used to get varous context)
 void computeSignatureVTs(const FunctionType *Ty, const Function *TargetFunc,
                          const Function &ContextFunc, const TargetMachine &TM,
                          SmallVectorImpl<MVT> &Params,

llvm/lib/Target/WebAssembly/WebAssemblyMachineFunctionInfo.cpp

 WebAssemblyFunctionInfo::~WebAssemblyFunctionInfo() = default; // anchor.
 
 void WebAssemblyFunctionInfo::initWARegs(MachineRegisterInfo &MRI) {
   assert(WARegs.empty());
   unsigned Reg = UnusedReg;
   WARegs.resize(MRI.getNumVirtRegs(), Reg);
 }
 
-void llvm::computeLegalValueVTs(const Function &F, const TargetMachine &TM,
+void llvm::computeLegalValueVTs(const WebAssemblyTargetLowering &TLI,
+                                LLVMContext &Ctx, const DataLayout &DL,
                                 Type *Ty, SmallVectorImpl<MVT> &ValueVTs) {
-  const DataLayout &DL(F.getParent()->getDataLayout());
-  const WebAssemblyTargetLowering &TLI =
-      *TM.getSubtarget<WebAssemblySubtarget>(F).getTargetLowering();
   SmallVector<EVT, 4> VTs;
   ComputeValueVTs(TLI, DL, Ty, VTs);
 
   for (EVT VT : VTs) {
-    unsigned NumRegs = TLI.getNumRegisters(F.getContext(), VT);
-    MVT RegisterVT = TLI.getRegisterType(F.getContext(), VT);
+    unsigned NumRegs = TLI.getNumRegisters(Ctx, VT);
+    MVT RegisterVT = TLI.getRegisterType(Ctx, VT);
     for (unsigned I = 0; I != NumRegs; ++I)
       ValueVTs.push_back(RegisterVT);
   }
 }
 
+void llvm::computeLegalValueVTs(const Function &F, const TargetMachine &TM,
+                                Type *Ty, SmallVectorImpl<MVT> &ValueVTs) {
+  const DataLayout &DL(F.getParent()->getDataLayout());
+  const WebAssemblyTargetLowering &TLI =
+      *TM.getSubtarget<WebAssemblySubtarget>(F).getTargetLowering();
+  computeLegalValueVTs(TLI, F.getContext(), DL, Ty, ValueVTs);
+}
+
 void llvm::computeSignatureVTs(const FunctionType *Ty,
                                const Function *TargetFunc,
                                const Function &ContextFunc,
                                const TargetMachine &TM,
                                SmallVectorImpl<MVT> &Params,
                                SmallVectorImpl<MVT> &Results) {
   computeLegalValueVTs(ContextFunc, TM, Ty->getReturnType(), Results);