[WebAssembly] TargetStreamer cleanup (NFC)

[aheejin]

Summary

• Unify mixed argument names (Symbol and Sym) to Sym
• Changed MCSymbolWasm* argument of emit*** functions to const MCSymbolWasm*. It seems not very intuitive that emit function in the streamer modifies symbol contents.
• Moved empty function bodies to the header
• clang-format

[aheejin]

More cleanups

[aheejin]

• Move more setType to AsmPrinter

[aheejin]

• clang-format & clang-tidy

[sbc100]

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp

137 ↗

(On Diff #176921)

We have two different streamers WebAssemblyTargetWasmStreamer and WebAssemblyTargetAsmStreamer. I seems that this code is only needed for the object file streamer so moving it out into common code is actually a semantic change.

Can you revert this part or at least put it in separate change so that this one can be a simple NFC?

[sbc100]

There is a presensent for the "emit" functions in the streamer modifying symbols. For example:

void MCELFStreamer::emitELFSize(MCSymbol *Symbol, const MCExpr *Value) {         
  cast<MCSymbolELF>(Symbol)->setSize(Value);                                     
}

I'm not saying this is the best, but it seems to have be designed or used this way in other targets, so I'd be tempted to use caution when refactoring. I myself don't have enough to background to know if this is a direction we want to move in. @sunfish and @aardappel might have a better idea.

[sbc100]

lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

115 ↗

(On Diff #176921)

I think it might make sense to continue to have emitImportModule to one thing for the asm case and the other for the binary streamer. Calling setModuleName() is clearly not needed for the asm writer so why do it?

Perhaps the difficulty comes from the fact that WebAssemblyTargetWasmStreamer wants to set attributes on the symbols when maybe it should be storing this "extra" symbol information that only it requires in a side data structure?

[aheejin]

In D55347#1321482, @sbc100 wrote:

There is a presensent for the "emit" functions in the streamer modifying symbols. For example:

void MCELFStreamer::emitELFSize(MCSymbol *Symbol, const MCExpr *Value) {         
  cast<MCSymbolELF>(Symbol)->setSize(Value);                                     
}

I'm not saying this is the best, but it seems to have be designed or used this way in other targets, so I'd be tempted to use caution when refactoring. I myself don't have enough to background to know if this is a direction we want to move in. @sunfish and @aardappel might have a better idea.

I checked streamers of other targets and it was kind of 50/50 (const / non-const). So I don't feel very strongly about putting const in there. And I thought MCSymbolWasm's type is a symbol's inherent property, so it is not necessarily be only assocaited with object files. WebAssemblyMCInstLower sets types to symbols even before does not know whether it will emit .s or .o file.

But anyway I don't feel strongly about this, so I can revert setType and setModuleName related part if you suggest so.

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp

137 ↗

(On Diff #176921)

If AsmStreamer does not use this property, isn't it NFC anyway? No tests can be affected.

[aheejin]

lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp

82 ↗

(On Diff #176921)

For emitGlobalType and emitEventType we check if the given symbol is global or event type even in AsmStreamer, which supports what I said before: Symbol types are just inherent properties of symbols and should not necessarily be tied to asm case vs. obj case. But if we try to add the same assertion to WebAssemblyTargetAsmStreamer::emitFunctionType above (which I did), it fails without my change in this CL that moves setType(wasm::WASM_SYMBOL_TYPE_FUNCTION) to AsmPrinter.

[aardappel]

We have 4 possible paths of emitting code (backend vs assembler, binary vs asm), so my answer for "where should this setType call go" is "wherever it makes those 4 paths most uniform / least fragile".

That said, I'd say having the streamer setting state on symbols feels somewhat weird to me, given that a streamer is mostly a dumb byte/text writer, and not responsible for managing state (symbols are owned by MCContext, outside of the streamer). But again, the above rule is more important to me :) We have a lot of places in the code that are somewhat weird, because we're a nonstandard "CPU", so I don't think that should be of primary concern.

[aheejin]

In D55347#1325860, @aardappel wrote:

We have 4 possible paths of emitting code (backend vs assembler, binary vs asm), so my answer for "where should this setType call go" is "wherever it makes those 4 paths most uniform / least fragile".

That said, I'd say having the streamer setting state on symbols feels somewhat weird to me, given that a streamer is mostly a dumb byte/text writer, and not responsible for managing state (symbols are owned by MCContext, outside of the streamer). But again, the above rule is more important to me :) We have a lot of places in the code that are somewhat weird, because we're a nonstandard "CPU", so I don't think that should be of primary concern.

This patch is trying to make those 4 paths more uniform. Some paths call setType during the code generation part before it reaches the streamer, and others call it during the streamer. What I suggest is, if we want to set symbols' properties (such as calling setType or setModuleName) within the streamer, we should do that for all paths. Otherwise we should do that outside of the streamer for all paths.

[aardappel]

Then yes, whichever you think is more uniform I am cool with.

[aheejin]

@sbc100 Do you have any concerns on landing this?

[sbc100]

If @aardappel and @dschuff are ok with this change I'll defer to them.

[aheejin]

@dschuff Do you think this looks OK?

[dschuff]

At a high level (i.e. across all the LLVM targets) the intention is that binary and asm streamers have the same interface. That's sort of enforced by the fact that they both have the same base class, but I guess in actuality the "interface" also includes the data on the symbol object. I think I agree that it makes more sense to think of the symbols as owned outside the streamer and have them be a way to pass info into the streamer (the current code is more like having them be state that is only needed by the binary streamer but which lives outside the streamer). I guess that's also another way of saying that symbol type and module name are properties of a symbol even if those properties are only needed by one of the streamers. I guess it might also be useful for debugging or future modifications to have the contents of the symbols be the same regardless of which output type is in use.

So... I guess that's a long-winded way of saying LGTM?

[aheejin]

Thank you all for comments!

llvm/trunk/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.h

 public:
   explicit WebAssemblyTargetStreamer(MCStreamer &S);
 
   /// .local
   virtual void emitLocal(ArrayRef<wasm::ValType> Types) = 0;
   /// .endfunc
   virtual void emitEndFunc() = 0;
   /// .functype
-  virtual void emitFunctionType(MCSymbolWasm *Symbol) = 0;
+  virtual void emitFunctionType(const MCSymbolWasm *Sym) = 0;
   /// .indidx
   virtual void emitIndIdx(const MCExpr *Value) = 0;
   /// .globaltype
-  virtual void emitGlobalType(MCSymbolWasm *Sym) = 0;
+  virtual void emitGlobalType(const MCSymbolWasm *Sym) = 0;
   /// .eventtype
-  virtual void emitEventType(MCSymbolWasm *Sym) = 0;
+  virtual void emitEventType(const MCSymbolWasm *Sym) = 0;
   /// .import_module
-  virtual void emitImportModule(MCSymbolWasm *Sym, StringRef ModuleName) = 0;
+  virtual void emitImportModule(const MCSymbolWasm *Sym,
+                                StringRef ModuleName) = 0;
 
 protected:
   void emitValueType(wasm::ValType Type);
 };
 
 /// This part is for ascii assembly output
 class WebAssemblyTargetAsmStreamer final : public WebAssemblyTargetStreamer {
   formatted_raw_ostream &OS;
 
 public:
   WebAssemblyTargetAsmStreamer(MCStreamer &S, formatted_raw_ostream &OS);
 
   void emitLocal(ArrayRef<wasm::ValType> Types) override;
   void emitEndFunc() override;
-  void emitFunctionType(MCSymbolWasm *Symbol) override;
+  void emitFunctionType(const MCSymbolWasm *Sym) override;
   void emitIndIdx(const MCExpr *Value) override;
-  void emitGlobalType(MCSymbolWasm *Sym) override;
-  void emitEventType(MCSymbolWasm *Sym) override;
-  void emitImportModule(MCSymbolWasm *Sym, StringRef ModuleName) override;
+  void emitGlobalType(const MCSymbolWasm *Sym) override;
+  void emitEventType(const MCSymbolWasm *Sym) override;
+  void emitImportModule(const MCSymbolWasm *Sym, StringRef ModuleName) override;
 };
 
 /// This part is for Wasm object output
 class WebAssemblyTargetWasmStreamer final : public WebAssemblyTargetStreamer {
 public:
   explicit WebAssemblyTargetWasmStreamer(MCStreamer &S);
 
   void emitLocal(ArrayRef<wasm::ValType> Types) override;
   void emitEndFunc() override;
-  void emitFunctionType(MCSymbolWasm *Symbol) override;
+  void emitFunctionType(const MCSymbolWasm *Sym) override {}
   void emitIndIdx(const MCExpr *Value) override;
-  void emitGlobalType(MCSymbolWasm *Sym) override;
-  void emitEventType(MCSymbolWasm *Sym) override;
-  void emitImportModule(MCSymbolWasm *Sym, StringRef ModuleName) override;
+  void emitGlobalType(const MCSymbolWasm *Sym) override {}
+  void emitEventType(const MCSymbolWasm *Sym) override {}
+  void emitImportModule(const MCSymbolWasm *Sym,
+                        StringRef ModuleName) override {}
 };
 
 /// This part is for null output
 class WebAssemblyTargetNullStreamer final : public WebAssemblyTargetStreamer {
 public:
   explicit WebAssemblyTargetNullStreamer(MCStreamer &S)
       : WebAssemblyTargetStreamer(S) {}
 
   void emitLocal(ArrayRef<wasm::ValType>) override {}
   void emitEndFunc() override {}
-  void emitFunctionType(MCSymbolWasm *) override {}
+  void emitFunctionType(const MCSymbolWasm *) override {}
   void emitIndIdx(const MCExpr *) override {}
-  void emitGlobalType(MCSymbolWasm *) override {}
-  void emitEventType(MCSymbolWasm *) override {}
-  void emitImportModule(MCSymbolWasm *, StringRef) override {}
+  void emitGlobalType(const MCSymbolWasm *) override {}
+  void emitEventType(const MCSymbolWasm *) override {}
+  void emitImportModule(const MCSymbolWasm *, StringRef) override {}
 };
 
 } // end namespace llvm
 
 #endif

llvm/trunk/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp

 
 WebAssemblyTargetAsmStreamer::WebAssemblyTargetAsmStreamer(
     MCStreamer &S, formatted_raw_ostream &OS)
     : WebAssemblyTargetStreamer(S), OS(OS) {}
 
 WebAssemblyTargetWasmStreamer::WebAssemblyTargetWasmStreamer(MCStreamer &S)
     : WebAssemblyTargetStreamer(S) {}
 
-static void PrintTypes(formatted_raw_ostream &OS, ArrayRef<wasm::ValType> Types) {
+static void printTypes(formatted_raw_ostream &OS,
+                       ArrayRef<wasm::ValType> Types) {
   bool First = true;
   for (auto Type : Types) {
     if (First)
       First = false;
     else
       OS << ", ";
     OS << WebAssembly::TypeToString(Type);
   }
   OS << '\n';
 }
 
 void WebAssemblyTargetAsmStreamer::emitLocal(ArrayRef<wasm::ValType> Types) {
   if (!Types.empty()) {
     OS << "\t.local  \t";
-    PrintTypes(OS, Types);
+    printTypes(OS, Types);
   }
 }
 
 void WebAssemblyTargetAsmStreamer::emitEndFunc() { OS << "\t.endfunc\n"; }
 
-void WebAssemblyTargetAsmStreamer::emitFunctionType(MCSymbolWasm *Symbol) {
-  OS << "\t.functype\t" << Symbol->getName() << " (";
-  auto &Params = Symbol->getSignature()->Params;
+void WebAssemblyTargetAsmStreamer::emitFunctionType(const MCSymbolWasm *Sym) {
+  assert(Sym->isFunction());
+  OS << "\t.functype\t" << Sym->getName() << " (";
+  auto &Params = Sym->getSignature()->Params;
   for (auto &Ty : Params) {
     if (&Ty != &Params[0]) OS << ", ";
     OS << WebAssembly::TypeToString(Ty);
   }
   OS << ") -> (";
-  auto &Returns = Symbol->getSignature()->Returns;
+  auto &Returns = Sym->getSignature()->Returns;
   for (auto &Ty : Returns) {
     if (&Ty != &Returns[0]) OS << ", ";
     OS << WebAssembly::TypeToString(Ty);
   }
   OS << ")\n";
 }
 
-void WebAssemblyTargetAsmStreamer::emitGlobalType(MCSymbolWasm *Sym) {
+void WebAssemblyTargetAsmStreamer::emitGlobalType(const MCSymbolWasm *Sym) {
   assert(Sym->isGlobal());
   OS << "\t.globaltype\t" << Sym->getName() << ", " <<
         WebAssembly::TypeToString(
           static_cast<wasm::ValType>(Sym->getGlobalType().Type)) <<
         '\n';
 }
 
-void WebAssemblyTargetAsmStreamer::emitEventType(MCSymbolWasm *Sym) {
+void WebAssemblyTargetAsmStreamer::emitEventType(const MCSymbolWasm *Sym) {
   assert(Sym->isEvent());
   OS << "\t.eventtype\t" << Sym->getName();
   if (Sym->getSignature()->Returns.empty())
     OS << ", void";
   else {
     assert(Sym->getSignature()->Returns.size() == 1);
     OS << ", "
        << WebAssembly::TypeToString(Sym->getSignature()->Returns.front());
   }
   for (auto Ty : Sym->getSignature()->Params)
     OS << ", " << WebAssembly::TypeToString(Ty);
   OS << '\n';
 }
 
-void WebAssemblyTargetAsmStreamer::emitImportModule(MCSymbolWasm *Sym,
+void WebAssemblyTargetAsmStreamer::emitImportModule(const MCSymbolWasm *Sym,
                                                     StringRef ModuleName) {
   OS << "\t.import_module\t" << Sym->getName() << ", " << ModuleName << '\n';
 }
 
 void WebAssemblyTargetAsmStreamer::emitIndIdx(const MCExpr *Value) {
   OS << "\t.indidx  \t" << *Value << '\n';
 }
 

 
 void WebAssemblyTargetWasmStreamer::emitEndFunc() {
   llvm_unreachable(".end_func is not needed for direct wasm output");
 }
 
 void WebAssemblyTargetWasmStreamer::emitIndIdx(const MCExpr *Value) {
   llvm_unreachable(".indidx encoding not yet implemented");
 }
-
-void WebAssemblyTargetWasmStreamer::emitFunctionType(MCSymbolWasm *Symbol) {
-  // Symbol already has its arguments and result set.
-  Symbol->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
-}
-
-void WebAssemblyTargetWasmStreamer::emitGlobalType(MCSymbolWasm *Sym) {
-  // Not needed.
-}
-
-void WebAssemblyTargetWasmStreamer::emitEventType(MCSymbolWasm *Sym) {
-  // Not needed.
-}
-void WebAssemblyTargetWasmStreamer::emitImportModule(MCSymbolWasm *Sym,
-                                                     StringRef ModuleName) {
-  Sym->setModuleName(ModuleName);
-}

llvm/trunk/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp

void WebAssemblyAsmPrinter::EmitEndOfAsmFile(Module &M) {
 
   for (const auto &F : M) {
     // Emit function type info for all undefined functions
     if (F.isDeclarationForLinker() && !F.isIntrinsic()) {
       SmallVector<MVT, 4> Results;
       SmallVector<MVT, 4> Params;
       ComputeSignatureVTs(F.getFunctionType(), F, TM, Params, Results);
       auto *Sym = cast<MCSymbolWasm>(getSymbol(&F));
+      Sym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
       if (!Sym->getSignature()) {
         auto Signature = SignatureFromMVTs(Results, Params);
         Sym->setSignature(Signature.get());
         addSignature(std::move(Signature));
       }
       // FIXME: this was originally intended for post-linking and was only used
       // for imports that were only called indirectly (i.e. s2wasm could not
       // infer the type from a call). With object files it applies to all
       // imports. so fix the names and the tests, or rethink how import
       // delcarations work in asm files.
       getTargetStreamer()->emitFunctionType(Sym);
 
       if (TM.getTargetTriple().isOSBinFormatWasm() &&
           F.hasFnAttribute("wasm-import-module")) {
         StringRef Name =
             F.getFnAttribute("wasm-import-module").getValueAsString();
+        Sym->setModuleName(Name);
         getTargetStreamer()->emitImportModule(Sym, Name);
       }
     }
   }
 
   for (const auto &G : M.globals()) {
     if (!G.hasInitializer() && G.hasExternalLinkage()) {
       if (G.getValueType()->isSized()) {
void WebAssemblyAsmPrinter::EmitFunctionBodyStart() {
   const Function &F = MF->getFunction();
   SmallVector<MVT, 1> ResultVTs;
   SmallVector<MVT, 4> ParamVTs;
   ComputeSignatureVTs(F.getFunctionType(), F, TM, ParamVTs, ResultVTs);
   auto Signature = SignatureFromMVTs(ResultVTs, ParamVTs);
   auto *WasmSym = cast<MCSymbolWasm>(CurrentFnSym);
   WasmSym->setSignature(Signature.get());
   addSignature(std::move(Signature));
+  WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
 
   // FIXME: clean up how params and results are emitted (use signatures)
   getTargetStreamer()->emitFunctionType(WasmSym);
 
   // Emit the function index.
   if (MDNode *Idx = F.getMetadata("wasm.index")) {
     assert(Idx->getNumOperands() == 1);