Page MenuHomePhabricator

[WebAssembly] Allow inlining functions with different features
ClosedPublic

Authored by tlively on Aug 6 2020, 8:24 PM.

Details

Summary

Because WebAssembly modules have to validate, having different feature
sets in different functions is not useful. As such, there is no reason
to prevent any function from being inlined into any other function due
to their enabled target features. This patch relaxes the inlining
restrictions for WebAssembly accordingly.

Frontends need to be careful, however, to set up their TargetMachines
with all the necessary features so that a feature will not be
"forgotten" because all the functions that used it were inlined and
removed from the module.

Requested in PR46812.

Diff Detail

Event Timeline

tlively created this revision.Aug 6 2020, 8:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2020, 8:24 PM
tlively requested review of this revision.Aug 6 2020, 8:24 PM

@alexcrichton As someone who works on an LLVM frontend, do you think the possibility that a feature can be "forgotten" would be worth introducing a restriction that functions can only be inlined into other functions that have a superset of their features?

Ah that's actually a good point. I think in that case it may be best to only inline into functions which have a superset of features. A pretty common expected use is where you enable simd128 in some functions and then call a bunch of intrinsics. The standard library itself won't be compiled with simd128 by default, so that'll be the only way to use simd128.

We could recognize that if any feature is used anywhere in a module we enable it for the module, but that would behave pretty wasm-specific compared to other targets, so it's probably best to do a subset check?

+1 on only inlining into functions which have a superset of features; it looks safer that way.

Sounds good, I'll make that change. @alexcrichton, would it be useful to have a separate LLVM pass that applies all features used anywhere in a module to each function in that module? Frontends could run that pass early when compiling to WebAssembly to remove the inlining restrictions.

Where are you planning to run that pass? Inlining runs within opt, before we can have any chances to run our own passes. You mean clang by 'frontend'? Can we run our own pass in clang?

Where are you planning to run that pass? Inlining runs within opt, before we can have any chances to run our own passes. You mean clang by 'frontend'? Can we run our own pass in clang?

Yeah, I was just looking at whether targets can run custom passes in clang. I didn't see any precedent for that, but it wouldn't be impossible.

Not sure if it's worth it if there's no precedents. If people use our toolchain (clang or emscripten or rust or whatever) that wouldn't happen in the first place. Maybe it'd be sufficient to print some warning messages in CoalesceFeaturesAndStripAtomics if features don't match..?

Yeah adding a pass to propagate the superset of all functions features to all functions sounds reasonable to me, it would be easy enough for that to be added to the pass manager on a wasm-specific basis.

tlively updated this revision to Diff 285466.Aug 13 2020, 12:32 PM
  • Require feature subsets
aheejin accepted this revision.Aug 13 2020, 1:06 PM
This revision is now accepted and ready to land.Aug 13 2020, 1:06 PM
This revision was landed with ongoing or failed builds.Aug 13 2020, 1:57 PM
This revision was automatically updated to reflect the committed changes.