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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:543
+  const Module *M = MMI->getModule();
+  emitExternalDecls(*M);
   assert(MF->getConstantPool()->getConstants().empty() &&
----------------
pmatos wrote:
> 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`.
Ho about we add a comment here...  something like "We use emitConstantPool to perform emitExternalDecls the first time its called so that the type checker sees all extarnal decls before any function bodies.   We probably should use `emitFunctionHeader` instead but that is not virtual".

Perhaps as a followup we could consider making `emitFunctionHeader` virtual?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115749