[WebAssembly] Remove unneeded MCSymbolRefExpr variants

[sbc100]

Summary

Also simplify WebAssemblyWasmObjectWriter::getRelocType.

We record the type of the symbol (event/function/data/global) in the
MCWasmSymbol and so it should always be clear how to handle a relocation
based on the symbol itself.

The exception is a function which still needs the special @TYPEINDEX
then the relocation contains the signature rather than the address
of the functions.

[sbc100]

• revert part

[sbc100]

@sunfish, what do you think? I'm not totally sure about how this part of the code is supposed to work, but this seems better to me.

[aheejin]

Looks OK to me, we have all the info in MCSymbolWasm after all.

llvm/lib/MC/WasmObjectWriter.cpp

1559

I guess this should be below if (!SymRef) test?

[dschuff]

IIRC this code goes back to when we were piggybacking on ELF, and is more-or-less there to match how those variants are used on ELF. I don't see any asm parser changes in this CL, but I could imagine that they would exist so that the assembler could know what kind of reloc to use even without any context or special knowledge of symbol types; does this change affect how asm is parsed? Maybe we've already added enough extra intelligence to the assembler that we don't need them.

[sbc100]

In D58472#1407342, @dschuff wrote:

IIRC this code goes back to when we were piggybacking on ELF, and is more-or-less there to match how those variants are used on ELF. I don't see any asm parser changes in this CL, but I could imagine that they would exist so that the assembler could know what kind of reloc to use even without any context or special knowledge of symbol types; does this change affect how asm is parsed? Maybe we've already added enough extra intelligence to the assembler that we don't need them.

You are correct. The @XXX is for the reloc type. When a symbol is referenced normally its just means that address of the symbol, but for things other than address (or normal refs) you need the @. That is why we still have the @TYPEINDEX for when you want the functions type, not its address. For any given symbol we already know the type and we can defined a default meaning when we the symbol. We don't need to decorate the default references IIUC.

[dschuff]

Makes sense. I think the thing that has changed compared to when we started is that symbol types like function, global, and event are more first-class in the wasm object format. Anyway, this change seems fine. I assume the asm parser already isn't using these annotations and we still round-trip?

[sbc100]

In D58472#1407531, @dschuff wrote:

Makes sense. I think the thing that has changed compared to when we started is that symbol types like function, global, and event are more first-class in the wasm object format. Anyway, this change seems fine. I assume the asm parser already isn't using these annotations and we still round-trip?

Right, the symbols types from the special ".functype" and ".eventtype" declarations I believe, not from the references.

llvm/include/llvm/MC/MCExpr.h

enum VariantKind : uint16_t {
     VK_Hexagon_GPREL,
     VK_Hexagon_GD_GOT,
     VK_Hexagon_LD_GOT,
     VK_Hexagon_GD_PLT,
     VK_Hexagon_LD_PLT,
     VK_Hexagon_IE,
     VK_Hexagon_IE_GOT,
 
-    VK_WebAssembly_FUNCTION, // Function table index, rather than virtual addr
-    VK_WebAssembly_GLOBAL,   // Global object index
-    VK_WebAssembly_TYPEINDEX,// Type table index
-    VK_WebAssembly_EVENT,    // Event index
+    VK_WebAssembly_TYPEINDEX,// Reference to a symbol's type (signature)
 
     VK_AMDGPU_GOTPCREL32_LO, // symbol@gotpcrel32@lo
     VK_AMDGPU_GOTPCREL32_HI, // symbol@gotpcrel32@hi
     VK_AMDGPU_REL32_LO,      // symbol@rel32@lo
     VK_AMDGPU_REL32_HI,      // symbol@rel32@hi
     VK_AMDGPU_REL64,         // symbol@rel64
 
     VK_TPREL,

llvm/lib/MC/MCExpr.cpp

StringRef MCSymbolRefExpr::getVariantKindName(VariantKind Kind) {
   case VK_Hexagon_HI16: return "HI16";
   case VK_Hexagon_GPREL: return "GPREL";
   case VK_Hexagon_GD_GOT: return "GDGOT";
   case VK_Hexagon_LD_GOT: return "LDGOT";
   case VK_Hexagon_GD_PLT: return "GDPLT";
   case VK_Hexagon_LD_PLT: return "LDPLT";
   case VK_Hexagon_IE: return "IE";
   case VK_Hexagon_IE_GOT: return "IEGOT";
-  case VK_WebAssembly_FUNCTION: return "FUNCTION";
-  case VK_WebAssembly_GLOBAL: return "GLOBAL";
   case VK_WebAssembly_TYPEINDEX: return "TYPEINDEX";
-  case VK_WebAssembly_EVENT: return "EVENT";
   case VK_AMDGPU_GOTPCREL32_LO: return "gotpcrel32@lo";
   case VK_AMDGPU_GOTPCREL32_HI: return "gotpcrel32@hi";
   case VK_AMDGPU_REL32_LO: return "rel32@lo";
   case VK_AMDGPU_REL32_HI: return "rel32@hi";
   case VK_AMDGPU_REL64: return "rel64";
   }
   llvm_unreachable("Invalid variant kind");
 }
return StringSwitch<VariantKind>(Name.lower())
     .Case("target1", VK_ARM_TARGET1)
     .Case("target2", VK_ARM_TARGET2)
     .Case("prel31", VK_ARM_PREL31)
     .Case("sbrel", VK_ARM_SBREL)
     .Case("tlsldo", VK_ARM_TLSLDO)
     .Case("lo8", VK_AVR_LO8)
     .Case("hi8", VK_AVR_HI8)
     .Case("hlo8", VK_AVR_HLO8)
-    .Case("function", VK_WebAssembly_FUNCTION)
-    .Case("global", VK_WebAssembly_GLOBAL)
     .Case("typeindex", VK_WebAssembly_TYPEINDEX)
-    .Case("event", VK_WebAssembly_EVENT)
     .Case("gotpcrel32@lo", VK_AMDGPU_GOTPCREL32_LO)
     .Case("gotpcrel32@hi", VK_AMDGPU_GOTPCREL32_HI)
     .Case("rel32@lo", VK_AMDGPU_REL32_LO)
     .Case("rel32@hi", VK_AMDGPU_REL32_HI)
     .Case("rel64", VK_AMDGPU_REL64)
     .Default(VK_Invalid);
 }
 

llvm/lib/MC/WasmObjectWriter.cpp

for (const uint8_t *
          P != End; ++P) {
       if (*P != 0)
         report_fatal_error("non-symbolic data in .init_array section");
     }
     for (const MCFixup &Fixup : DataFrag.getFixups()) {
       assert(Fixup.getKind() ==
              MCFixup::getKindForSize(is64Bit() ? 8 : 4, false));
       const MCExpr *Expr = Fixup.getValue();
-      auto *Sym = dyn_cast<MCSymbolRefExpr>(Expr);
-      if (!Sym)
+      auto *SymRef = dyn_cast<MCSymbolRefExpr>(Expr);
+      const auto &TargetSym = cast<const MCSymbolWasm>(SymRef->getSymbol());
+      if (!SymRef)
         report_fatal_error("fixups in .init_array should be symbol references");
-      if (Sym->getKind() != MCSymbolRefExpr::VK_WebAssembly_FUNCTION)
-        report_fatal_error("symbols in .init_array should be for functions");
-      if (Sym->getSymbol().getIndex() == InvalidIndex)
+      if (TargetSym.getIndex() == InvalidIndex)
         report_fatal_error("symbols in .init_array should exist in symbtab");
+      if (!TargetSym.isFunction())
+        report_fatal_error("symbols in .init_array should be for functions");
       InitFuncs.push_back(
-          std::make_pair(Priority, Sym->getSymbol().getIndex()));
+          std::make_pair(Priority, TargetSym.getIndex()));
     }
   }
 
   // Write out the Wasm header.
   writeHeader(Asm);
 
   writeTypeSection(Signatures);
   writeImportSection(Imports, DataSize, TableElems.size());

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyWasmObjectWriter.cpp

private:
   unsigned getRelocType(const MCValue &Target,
                         const MCFixup &Fixup) const override;
 };
 } // end anonymous namespace
 
 WebAssemblyWasmObjectWriter::WebAssemblyWasmObjectWriter(bool Is64Bit)
     : MCWasmObjectTargetWriter(Is64Bit) {}
 
-// Test whether the given expression computes a function address.
-static bool isFunctionExpr(const MCExpr *Expr) {
-  if (auto SyExp = dyn_cast<MCSymbolRefExpr>(Expr))
-    return cast<MCSymbolWasm>(SyExp->getSymbol()).isFunction();
-
-  if (auto BinOp = dyn_cast<MCBinaryExpr>(Expr))
-    return isFunctionExpr(BinOp->getLHS()) != isFunctionExpr(BinOp->getRHS());
-
-  if (auto UnOp = dyn_cast<MCUnaryExpr>(Expr))
-    return isFunctionExpr(UnOp->getSubExpr());
-
-  return false;
-}
-
-static bool isFunctionType(const MCValue &Target) {
-  const MCSymbolRefExpr *RefA = Target.getSymA();
-  return RefA && RefA->getKind() == MCSymbolRefExpr::VK_WebAssembly_TYPEINDEX;
+static bool isFunctionSignatureRef(const MCSymbolRefExpr *Ref) {
+  return Ref->getKind() == MCSymbolRefExpr::VK_WebAssembly_TYPEINDEX;
 }
 
 static const MCSection *getFixupSection(const MCExpr *Expr) {
   if (auto SyExp = dyn_cast<MCSymbolRefExpr>(Expr)) {
     if (SyExp->getSymbol().isInSection())
       return &SyExp->getSymbol().getSection();
     return nullptr;
   }
 
   if (auto BinOp = dyn_cast<MCBinaryExpr>(Expr)) {
     auto SectionLHS = getFixupSection(BinOp->getLHS());
     auto SectionRHS = getFixupSection(BinOp->getRHS());
     return SectionLHS == SectionRHS ? nullptr : SectionLHS;
   }
 
   if (auto UnOp = dyn_cast<MCUnaryExpr>(Expr))
     return getFixupSection(UnOp->getSubExpr());
 
   return nullptr;
 }
 
-static bool isGlobalType(const MCValue &Target) {
-  const MCSymbolRefExpr *RefA = Target.getSymA();
-  return RefA && RefA->getKind() == MCSymbolRefExpr::VK_WebAssembly_GLOBAL;
-}
-
-static bool isEventType(const MCValue &Target) {
-  const MCSymbolRefExpr *RefA = Target.getSymA();
-  return RefA && RefA->getKind() == MCSymbolRefExpr::VK_WebAssembly_EVENT;
-}
-
 unsigned WebAssemblyWasmObjectWriter::getRelocType(const MCValue &Target,
                                                    const MCFixup &Fixup) const {
-  // WebAssembly functions are not allocated in the data address space. To
-  // resolve a pointer to a function, we must use a special relocation type.
-  bool IsFunction = isFunctionExpr(Fixup.getValue());
+  const MCSymbolRefExpr *RefA = Target.getSymA();
+  assert(RefA);
+  auto& SymA = cast<MCSymbolWasm>(RefA->getSymbol());
 
   switch (unsigned(Fixup.getKind())) {
   case WebAssembly::fixup_code_sleb128_i32:
-    if (IsFunction)
+    if (SymA.isFunction())
       return wasm::R_WASM_TABLE_INDEX_SLEB;
     return wasm::R_WASM_MEMORY_ADDR_SLEB;
   case WebAssembly::fixup_code_sleb128_i64:
     llvm_unreachable("fixup_sleb128_i64 not implemented yet");
   case WebAssembly::fixup_code_uleb128_i32:
-    if (isGlobalType(Target))
-      return wasm::R_WASM_GLOBAL_INDEX_LEB;
-    if (isFunctionType(Target))
+    if (SymA.isFunction()) {
+      if (isFunctionSignatureRef(RefA))
         return wasm::R_WASM_TYPE_INDEX_LEB;
-    if (IsFunction)
+      else
         return wasm::R_WASM_FUNCTION_INDEX_LEB;
-    if (isEventType(Target))
+    }
+    if (SymA.isGlobal())
+      return wasm::R_WASM_GLOBAL_INDEX_LEB;
+    if (SymA.isEvent())
       return wasm::R_WASM_EVENT_INDEX_LEB;
     return wasm::R_WASM_MEMORY_ADDR_LEB;
   case FK_Data_4:
-    if (IsFunction)
+    if (SymA.isFunction())
       return wasm::R_WASM_TABLE_INDEX_I32;
     if (auto Section = static_cast<const MCSectionWasm *>(
             getFixupSection(Fixup.getValue()))) {
       if (Section->getKind().isText())
         return wasm::R_WASM_FUNCTION_OFFSET_I32;
       else if (!Section->isWasmData())
         return wasm::R_WASM_SECTION_OFFSET_I32;
     }

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h

public:
   //===------------------------------------------------------------------===//
 
   void EmitEndOfAsmFile(Module &M) override;
   void EmitProducerInfo(Module &M);
   void EmitJumpTableInfo() override;
   void EmitConstantPool() override;
   void EmitFunctionBodyStart() override;
   void EmitInstruction(const MachineInstr *MI) override;
-  const MCExpr *lowerConstant(const Constant *CV) override;
   bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                        unsigned AsmVariant, const char *ExtraCode,
                        raw_ostream &OS) override;
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
                              unsigned AsmVariant, const char *ExtraCode,
                              raw_ostream &OS) override;
 
   MVT getRegType(unsigned RegNo) const;
   std::string regToString(const MachineOperand &MO);
   WebAssemblyTargetStreamer *getTargetStreamer();
 };
 
 } // end namespace llvm
 
 #endif

llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

default: {
     MCInst TmpInst;
     MCInstLowering.lower(MI, TmpInst);
     EmitToStreamer(*OutStreamer, TmpInst);
     break;
   }
   }
 }
 
-const MCExpr *WebAssemblyAsmPrinter::lowerConstant(const Constant *CV) {
-  if (const auto *GV = dyn_cast<GlobalValue>(CV))
-    if (GV->getValueType()->isFunctionTy()) {
-      return MCSymbolRefExpr::create(
-          getSymbol(GV), MCSymbolRefExpr::VK_WebAssembly_FUNCTION, OutContext);
-    }
-  return AsmPrinter::lowerConstant(CV);
-}
-
 bool WebAssemblyAsmPrinter::PrintAsmOperand(const MachineInstr *MI,
                                             unsigned OpNo, unsigned AsmVariant,
                                             const char *ExtraCode,
                                             raw_ostream &OS) {
   if (AsmVariant != 0)
     report_fatal_error("There are no defined alternate asm variants");
 
   // First try the generic code, which knows about modifiers like 'c' and 'n'.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

MCSymbol *WebAssemblyMCInstLower::GetExternalSymbolSymbol(
 
   return WasmSym;
 }
 
 MCOperand WebAssemblyMCInstLower::lowerSymbolOperand(MCSymbol *Sym,
                                                      int64_t Offset,
                                                      bool IsFunc, bool IsGlob,
                                                      bool IsEvent) const {
-  MCSymbolRefExpr::VariantKind VK =
-      IsFunc ? MCSymbolRefExpr::VK_WebAssembly_FUNCTION
-             : IsGlob ? MCSymbolRefExpr::VK_WebAssembly_GLOBAL
-                      : IsEvent ? MCSymbolRefExpr::VK_WebAssembly_EVENT
-                                : MCSymbolRefExpr::VK_None;
-
-  const MCExpr *Expr = MCSymbolRefExpr::create(Sym, VK, Ctx);
+  const MCExpr *Expr =
+      MCSymbolRefExpr::create(Sym, MCSymbolRefExpr::VK_None, Ctx);
 
   if (Offset != 0) {
     if (IsFunc)
       report_fatal_error("Function addresses with offsets not supported");
     if (IsGlob)
       report_fatal_error("Global indexes with offsets not supported");
     if (IsEvent)
       report_fatal_error("Event indexes with offsets not supported");

llvm/test/MC/WebAssembly/annotations.s

test_annotation:
   catch
   block
   try
   br        0
   catch
   local.set 0
   block    i32
   local.get 0
-  br_on_exn 0, __cpp_exception@EVENT
+  br_on_exn 0, __cpp_exception
   rethrow
   end_block
   end_try
   end_block
   rethrow
   end_try
   end_try
   end_function

 # CHECK-NEXT:   catch                     # catch1:
 # CHECK-NEXT:   block
 # CHECK-NEXT:   try
 # CHECK-NEXT:   br        0               # 0: down to label5
 # CHECK-NEXT:   catch                     # catch2:
 # CHECK-NEXT:   local.set 0
 # CHECK-NEXT:   block    i32
 # CHECK-NEXT:   local.get 0
-# CHECK-NEXT:   br_on_exn 0, __cpp_exception@EVENT # 0: down to label6
+# CHECK-NEXT:   br_on_exn 0, __cpp_exception # 0: down to label6
 # CHECK-NEXT:   rethrow                   # to caller
 # CHECK-NEXT:   end_block                 # label6:
 # CHECK-NEXT:   end_try                   # label5:
 # CHECK-NEXT:   end_block                 # label4:
 # CHECK-NEXT:   rethrow                   # to caller
 # CHECK-NEXT:   end_try                   # label3:
 # CHECK-NEXT:   end_try                   # label0:
 # CHECK-NEXT:   end_function
 

llvm/test/MC/WebAssembly/assembler-binary.ll

 ; Checking assembly is not the point of this test, but if something breaks
 ; it is easier to spot it here than in the yaml output.
 
 ; ASM:      .text
 ; ASM:        .file "assembler-binary.ll"
 ; ASM:        .globl  foo
 ; ASM:      foo:
 ; ASM-NEXT:   .functype foo (i32) -> ()
-; ASM-NEXT:   call  bar@FUNCTION
+; ASM-NEXT:   call  bar
 ; ASM-NEXT:   end_function
 ; ASM:        .functype bar () -> ()
 
 
 ; CHECK:      --- !WASM
 ; CHECK-NEXT: FileHeader:
 ; CHECK-NEXT:   Version:         0x00000001
 ; CHECK-NEXT: Sections:

llvm/test/MC/WebAssembly/basic-assembly.s

test0:
     # Loops, conditionals, binary ops, calls etc:
     block       i32
     i32.const   1
     local.get   0
     i32.ge_s
     br_if       0        # 0: down to label0
 .LBB0_1:
     loop        i32      # label1:
-    call        something1@FUNCTION
+    call        something1
     i64.const   1234
-    i32.call    something2@FUNCTION
+    i32.call    something2
     i32.const   0
     call_indirect 0
     i32.const   1
     i32.add
     local.tee   0
     local.get   0
     i32.lt_s
     br_if       0        # 0: up to label1
.LBB0_2:
     #i32x4.trunc_sat_f32x4_s
     i32.trunc_f32_s
     try         except_ref
 .LBB0_3:
     catch
     local.set 0
     block       i32
     local.get 0
-    br_on_exn 0, __cpp_exception@EVENT
+    br_on_exn 0, __cpp_exception
     rethrow
 .LBB0_4:
     end_block
     end_try
     i32.const 0
     throw 0
 .LBB0_5:
     #i32.trunc_sat_f32_s
-    global.get  __stack_pointer@GLOBAL
+    global.get  __stack_pointer
     end_function
     .globaltype __stack_pointer, i32
 
 # CHECK:           .text
 # CHECK-LABEL: test0:
 # CHECK-NEXT:      .functype test0 (i32, i64) -> (i32)
 # CHECK-NEXT:      .eventtype  __cpp_exception i32
 # CHECK-NEXT:      .local      f32, f64
 # CHECK-NEXT:      local.get   2
 # CHECK-NEXT:      local.set   2
 # CHECK-NEXT:      i32.const   -1
 # CHECK-NEXT:      f64.const   0x1.999999999999ap1
 # CHECK-NEXT:      v128.const  0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15
 # CHECK-NEXT:      v128.const  0, 1, 2, 3, 4, 5, 6, 7
 # CHECK-NEXT:      local.get   0
 # CHECK-NEXT:      f64.store   0:p2align=0
 # CHECK-NEXT:      block       i32
 # CHECK-NEXT:      i32.const   1
 # CHECK-NEXT:      local.get   0
 # CHECK-NEXT:      i32.ge_s
 # CHECK-NEXT:      br_if 0            # 0: down to label0
 # CHECK-NEXT:  .LBB0_1:
 # CHECK-NEXT:      loop        i32         # label1:
-# CHECK-NEXT:      call        something1@FUNCTION
+# CHECK-NEXT:      call        something1
 # CHECK-NEXT:      i64.const   1234
-# CHECK-NEXT:      i32.call    something2@FUNCTION
+# CHECK-NEXT:      i32.call    something2
 # CHECK-NEXT:      i32.const   0
 # CHECK-NEXT:      call_indirect 0
 # CHECK-NEXT:      i32.const   1
 # CHECK-NEXT:      i32.add
 # CHECK-NEXT:      local.tee   0
 # CHECK-NEXT:      local.get   0
 # CHECK-NEXT:      i32.lt_s
 # CHECK-NEXT:      br_if 0            # 0: up to label1

 # CHECK-NEXT:      f32x4.add
 # CHECK-NEXT:      i32.trunc_f32_s
 # CHECK-NEXT:      try         except_ref
 # CHECK-NEXT:  .LBB0_3:
 # CHECK-NEXT:      catch
 # CHECK-NEXT:      local.set 0
 # CHECK-NEXT:      block       i32
 # CHECK-NEXT:      local.get 0
-# CHECK-NEXT:      br_on_exn 0, __cpp_exception@EVENT
+# CHECK-NEXT:      br_on_exn 0, __cpp_exception
 # CHECK-NEXT:      rethrow
 # CHECK-NEXT:  .LBB0_4:
 # CHECK-NEXT:      end_block
 # CHECK-NEXT:      end_try
 # CHECK-NEXT:      i32.const 0
 # CHECK-NEXT:      throw 0
 # CHECK-NEXT:  .LBB0_5:
-# CHECK-NEXT:      global.get  __stack_pointer@GLOBAL
+# CHECK-NEXT:      global.get  __stack_pointer
 # CHECK-NEXT:      end_function
 
 # CHECK:           .globaltype  __stack_pointer, i32