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

[Paulo Matos via Phabricator via llvm-commits]

pmatos added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:290
     return;
   signaturesEmitted = true;
 
----------------
sbc100 wrote:
> Is this re-entrancy check still needed?  i.e. maybe we now just call `emitExternalDecls` once? 
Yes, the reentrancy check is still needed. The function `emitConstantPool` is called once for each function when emitting function headers. We want to emit this before the first function.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:543
+  const Module *M = MMI->getModule();
+  emitExternalDecls(*M);
   assert(MF->getConstantPool()->getConstants().empty() &&
----------------
sbc100 wrote:
> 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?
Yes, unfortunately, it would have been much better to override `emitFunctionHeader` however that function is not virtual. So the candidates become `emitConstantPool` or (called a bit later) `emitFunctionEntryLabel`.  The later is not great thought because some visibility/linkage directives for the function might be emitted before the label and we might end up mixing declaration of globals with function directives which doesn't look so good. Therefore, we are left with doing this in `emitConstantPool`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115749