This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Split WebAssemblyUtils to fix library layering for MC tools.
ClosedPublic

Authored by craig.topper on Feb 19 2023, 1:48 PM.

Details

Summary

WebAssemblyUtils depends on CodeGen which depends on all middle end
optimization libraries.

This component is used by WebAssembly's AsmParser, Disassembler, and
MCTargetDesc libraries. Because of this, any MC layer tool built with
WebAssembly support includes a larger portion of LLVM than it should.

To fix this I've created MC only versions of WebAssemblyUtilties.cpp
and WebAssemblyTypeUtilities.cpp in MCTargetDesc to be used by the MC
components.

This shrinks llvm-objdump and llvm-mc on my local release+asserts
build by 5-6 MB.

This is a pretty naive splitting so I welcome feedback on how
WebAssembly developers would like to see this structured.

Diff Detail

Event Timeline

craig.topper created this revision.Feb 19 2023, 1:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2023, 1:48 PM
craig.topper requested review of this revision.Feb 19 2023, 1:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2023, 1:48 PM
craig.topper edited the summary of this revision. (Show Details)Feb 19 2023, 1:49 PM

The general idea of this makes sense to me.
@pmatos and @asb have been thinking more about the type system recently, they might have an opinion.

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCUtilities.cpp
21 ↗(On Diff #498694)

@aheejin these should maybe be on the CodeGen side rather than on the MC side, right?

Thanks! The idea makes sense to me.

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCUtilities.cpp
21 ↗(On Diff #498694)

Yeah, I don't think this file is very relevant to MC.

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCUtilities.h
1 ↗(On Diff #498694)

It looks MCTargetDesc/WebAssemblyMCUtilities.h and MCTargetDesc/WebAssemblyMCUtilities.cpp don't have any MC-related functionalities. Why do they need to be moved into MCTargetDesc/?

craig.topper added inline comments.Feb 22 2023, 7:38 PM
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCUtilities.h
1 ↗(On Diff #498694)

Two of the options are used here.

WebAssemblyMCAsmInfo.cpp
53:  if (WebAssembly::WasmEnableEH || WebAssembly::WasmEnableSjLj)

I didn't look too closely to notice the other two aren't used in MC.

There are some missing dependencies in a -DBUILD_SHARED_LIBS=on build.

The change looks good.

... This results in any MC layer tool built with WebAssembly including a larger portion of LLVM that it should.

s/should/shouldn't/

craig.topper edited the summary of this revision. (Show Details)Feb 23 2023, 10:27 AM

Make WebAssembly AsmParser depended on WebAssemblyDesc

MaskRay accepted this revision.Feb 23 2023, 11:13 AM

LGTM. Note that there is unresolved comment about enable-emscripten-cxx-exceptions and its friends belonging to the CodeGen part rather than the MC part.

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCUtilities.cpp
34 ↗(On Diff #499920)

Drop , cl::init(false) (default)

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCUtilities.h
20 ↗(On Diff #499920)

With C++17 we can use namespace llvm::WebAssembly

This revision is now accepted and ready to land.Feb 23 2023, 11:13 AM

-DBUILD_SHARED_LIBS=on builds still fail.

[2540/2713] Linking CXX shared library lib/libLLVMWebAssemblyUtils.so.17git
FAILED: lib/libLLVMWebAssemblyUtils.so.17git

-DBUILD_SHARED_LIBS=on builds still fail.

[2540/2713] Linking CXX shared library lib/libLLVMWebAssemblyUtils.so.17git
FAILED: lib/libLLVMWebAssemblyUtils.so.17git

Thanks. I fixed one problem and then went back to the wrong build directory. Rebuilding the shared build now.

Fix -DBUILD_SHARED_LIBS=on

Address review comments

aheejin added inline comments.Feb 23 2023, 3:16 PM
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCUtilities.h
1 ↗(On Diff #498694)

Ah, right... That was added in D115893, and the reason we needed that in MCAsmInfo was we couldn't determine the EH mode only with the triple. Yeah, this is not very ideal, but I can't think of any other way around this atm. Sorry for making you revert the change, but can we bring back the two other options (WasmEnableEmEH and WasmEnableEmSjLj) here? I think it's clearer to gather all EH/SjLj options in one place. Also given that this file only contains options, can we change the file name to, say, WebAssemblyEHSjLjOptions.cpp/h?

Move the commannd line options to WebAssemblyMCTargetDesc.cpp/h. 4 command line options didn't seem to warrant a new file and this header is already included pretty heavily.

aheejin accepted this revision.Feb 23 2023, 8:15 PM

Thank you!