[PATCH] D92840: [WebAssembly] call_indirect causes indirect function table import

[Andy Wingo via Phabricator via llvm-commits]

wingo added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:167
+  // which itself duplicates MC/WasmObjectWriter; rework to factor out a utils
+  // component?
+  MCSymbolWasm *Sym = cast_or_null<MCSymbolWasm>(Ctx.lookupSymbol(Name));
----------------
aheejin wrote:
> wingo wrote:
> > wingo wrote:
> > > aheejin wrote:
> > > > wingo wrote:
> > > > > aheejin wrote:
> > > > > > I'm not sure if there's an easy way to factor out so all three places can use it, but at least can't we use WebAssemblyUtilities.cpp's `getOrCreateFunctionTableSymbol` here and not define it again? Can't we do `#include "WebAssemblyUtilities.h" and use its function?
> > > > > Sadly we can't directly re-use it here, for reasons I don't fully understand.  If you #include the WebAssemblyUtilities.h file, it compiles fine but `llvm-mc` fails to link.  It would seem that one might need to factor WebAssemblyUtilities to a separate cmake component.
> > > > Does adding `WebAssemblyCodeGen` under `LINK_COMPONENTS` in [[ https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/WebAssembly/AsmParser/CMakeLists.txt | llvm/lib/Target/WebAssembly/AsmParser/CMakeLists.txt ]] work?
> > > I'll give it a try, but note fwiw that other `Target/$foo/AsmParser/CMakeLists.txt` don't include the codegen components
> > OK so this works :)  It will lead to more work for the linker though.  @aheejin I can factor out a `Target/WebAssembly/Utils/` dir and library component, as e.g. AArch64 and RISCV have, or I can just add WebAssemblyCodeGen to the LINK_COMPONENTS.  Do you have a preference?
> Making `Utils` directory sounds good to me. As you said, there seems to be already four targets which does that: AArch64, AMDGPU, ARM, and RISCV.
> 
> Maybe it would be good to land this as is and do the `Utils` refactoring as a followup CL.
Really sorry for the noise here but given that the existing WebAssemblyUtilities pulls in all of CodeGen anyway via use of MachineInstr in the other utility routines, it would appear to be just useless noodling to put it in Utils.  I will just make AsmParser use WebAssemblyCodeGen and call it a day.  Further feedback still welcome of course!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92840