[PATCH] D115749: [WebAssembly] Fix linkage and visibility info of IR global syms

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

I general,g this seems like a good change..

I wonder if you could split out the emitLinkage -> emitConstant pool change?  That would hopefully capture a lot of the test churn and seems maybe logically separable?



================
Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyTypeUtilities.h:111
+/// Sets WasmSymbol Type
+void WasmSymbolSetType(MCSymbolWasm *Sym, const Type *GlobalVT,
+                       const SmallVector<MVT, 1> &VTs);
----------------
The other functions in this file start with a lowercase letter..  


================
Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyTypeUtilities.h:111
+/// Sets WasmSymbol Type
+void WasmSymbolSetType(MCSymbolWasm *Sym, const Type *GlobalVT,
+                       const SmallVector<MVT, 1> &VTs);
----------------
sbc100 wrote:
> The other functions in this file start with a lowercase letter..  
Is this the right place for this function to live?  I don't see any other MC stuff in this file.


================
Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp:19
 #include "llvm/MC/MCContext.h"
+#include "llvm/Support/MachineValueType.h"
+
----------------
Is this change needed?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:290
     return;
   signaturesEmitted = true;
 
----------------
Is this re-entrancy check still needed?  i.e. maybe we now just call `emitExternalDecls` once? 


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:543
+  const Module *M = MMI->getModule();
+  emitExternalDecls(*M);
   assert(MF->getConstantPool()->getConstants().empty() &&
----------------
It seems odd to do anything substantive here, but despite the fact that "WebAssembly disables constant pools".

I guess we you choose this point because its happens to occur at the correct point in the asm printing process..  and maybe there is not other hook we can/should use at that point?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115749/new/

https://reviews.llvm.org/D115749