This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Put utility functions in Utils directory (NFC)
ClosedPublic

Authored by aheejin on Apr 21 2021, 1:19 PM.

Details

Summary

This CL

  1. Creates Utils/ directory under lib/Target/WebAssembly
  2. Moves existing WebAssemblyUtilities.cpp|h into the Utils/ directory
  3. Creates Utils/WebAssemblyTypeUtilities.cpp|h and put type declarataions and type conversion functions scattered in various places into this single place.

It has been suggested several times that it is not easy to share utility
functions between subdirectories (AsmParser, DIsassembler, MCTargetDesc,
...). Sometimes we ended up duplicating the same function because of
this.

There are already other targets doing this: AArch64, AMDGPU, and ARM
have Utils/ subdirectory under their target directory.

This extracts the utility functions into a single directory Utils/ and
make them sharable among all passes in WebAssembly/ and its
subdirectories. Also I believe gathering all type-related conversion
functionalities into a single place makes it more usable. (Actually I
was working on another CL that uses various type conversion functions
scattered in multiple places, which became the motivation for this CL.)

Diff Detail

Event Timeline

aheejin created this revision.Apr 21 2021, 1:19 PM
aheejin requested review of this revision.Apr 21 2021, 1:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2021, 1:19 PM
aheejin edited the summary of this revision. (Show Details)Apr 21 2021, 1:21 PM

@wingo, I wanted to merge WebAssembly::getOrCreateFunctionTableSymbol in WebAssemblyUtilities.cpp and GetOrCreateFunctionTableSymbol in WebAssemblyAsmParser.cpp and put it in Utils/WebAssemblyUtilities.cpp, but it looks they became diverged in D90948 and are not the same anymore. Can they still be merged?

aheejin updated this revision to Diff 339371.Apr 21 2021, 1:41 PM

Add _UTILS_ in ifndef/define macros

dschuff accepted this revision.Apr 21 2021, 2:21 PM

This looks good. Since you've created new build components, don't forget to test the LLVM/clang build and make sure it works with static libraries and with the LLVM dylib before you land.

This revision is now accepted and ready to land.Apr 21 2021, 2:21 PM
aheejin updated this revision to Diff 339428.Apr 21 2021, 5:22 PM

clang-tidy fix

@wingo, I wanted to merge WebAssembly::getOrCreateFunctionTableSymbol in WebAssemblyUtilities.cpp and GetOrCreateFunctionTableSymbol in WebAssemblyAsmParser.cpp and put it in Utils/WebAssemblyUtilities.cpp, but it looks they became diverged in D90948 and are not the same anymore. Can they still be merged?

Excellent initiative, you are more valiant than I was! Yes these can be merged, with a little tweak. Perhaps like this:

enum class SymbolCompat { None, OmitFromLinkingSection };

MCSymbolWasm *WebAssembly::getOrCreateFunctionTableSymbol(
    MCContext &Ctx, const StringRef &Name,
    SymbolCompat Compat = SymbolCompat::None) {
  MCSymbolWasm *Sym = cast_or_null<MCSymbolWasm>(Ctx.lookupSymbol(Name));
  if (Sym) {
    if (!Sym->isFunctionTable())
      Ctx.reportError(SMLoc(), "symbol is not a wasm funcref table");
  } else {
    Sym = cast<MCSymbolWasm>(Ctx.getOrCreateSymbol(Name));
    Sym->setFunctionTable();
    // The default function table is synthesized by the linker.
    Sym->setUndefined();
  }
  if (Compat == SymbolCompat::OmitFromLinkingSection)
    Sym->setOmitFromLinkingSection();
  return Sym;
}

MCSymbolWasm *WebAssembly::getOrCreateFunctionTableSymbol(
    MCContext &Ctx, const WebAssemblySubtarget *Subtarget) {
  StringRef Name = "__indirect_function_table";
  SymbolCompat Compat = SymbolCompat::None;
  // MVP object files can't have symtab entries for tables.
  if !(Subtarget && Subtarget->hasReferenceTypes()))
    Compat = SymbolCompat::OmitFromLinkingSection;
  return getOrCreateFunctionTableSymbol(Ctx, Name, Compat);
}

Then change the caller in AsmParser to s/GetOrCreateFunctionTableSymbol/WebAssembly::getOrCreateFunctionTableSymbol/.

aheejin updated this revision to Diff 339501.Apr 22 2021, 12:45 AM

Correct clang-tidy fix

aardappel accepted this revision.Apr 22 2021, 11:08 AM

Thanks for doing this, was sorely needed :)

aheejin added a comment.EditedApr 22 2021, 3:28 PM

@wingo Thanks! I don't have enough understanding about that part of the code myself, so if you think it's necessary please consider uploading a CL to merge that :)

thakis added a subscriber: thakis.Apr 22 2021, 7:27 PM
thakis added inline comments.
llvm/lib/Target/WebAssembly/Utils/CMakeLists.txt
7

Is this one needed?

Also, similar to aarch64, this is a bit awkward from a dependency perspective: The files in Utils include headers in Target/WebAssembly which include headers in Target/WebAssembly/MCTargetDesc, which include tablegen-generated headers. So Utils depends on generated headers, and kind of depends on MCTargetDesc, but MCTargetDesc also depends on Utils. It happens to work out, but it's a bit awkward.

Also, similar to aarch64, this is a bit awkward from a dependency perspective: The files in Utils include headers in Target/WebAssembly which include headers in Target/WebAssembly/MCTargetDesc, which include tablegen-generated headers. So Utils depends on generated headers, and kind of depends on MCTargetDesc, but MCTargetDesc also depends on Utils. It happens to work out, but it's a bit awkward.

Didn't that header file dependency already exist before: WebAssembly/MCTargetDesc/ depends on tablegen-generated headers in WebAssembly/ and WebAssembly/ depends on methods in WebAssembly/MCTargetDesc. So we already had header-file dependency of WebAssembly/ <-> WebAssembly/MCTargetDesc/. This worked, but we couldn't make this cyclic dependency work for linking, so this CL takes the linking dependency out to Utils/.

llvm/lib/Target/WebAssembly/Utils/CMakeLists.txt
7