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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin 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));
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92840