[NFC] Move InstrumentedPass logic out and use it in another place #6132

kripken · 2023-11-23T00:38:19Z

Asyncify gained a way to wrap a pass so that it only runs on a given set of
functions, rather than on all functions, so the wrapper "filters" what the pass
operates on. That was useful in Asyncify as we wanted to only do work on
functions that Asyncify actually instrumented.

There is another place in the code that needs such functionality,
optimizeAfterInlining, which runs optimizations after we inline; again, we
only want to optimize on the functions we know are relevant because they
changed. To do that, move that logic out to a general place so it can be
reused. This makes the code there a lot less hackish.

While doing so make the logic only work on function-parallel passes. It
never did anyhow, but now it asserts on that. (It can't run on a general
pass because a general one does not provide an interface to affect which
functions it operates on; a general pass is entirely opaque in that way.)

kripken · 2023-11-27T22:27:35Z

src/passes/pass-utils.h

+  }
+
+  void runOnFunction(Module* module, Function* func) override {
+    if (!relevantFuncs.count(func)) {


To verify this PR is correct (on top of the test suite having no changes), that is, that it actually runs only on the filtered functions, I checked what happens when this condition is flipped so that we operate on the inverse set of the functions we should. As expected the test suite then gets some huge diffs in asyncify and inlining tests.

tlively · 2023-11-28T01:57:24Z

src/passes/opt-utils.h

-  }
-  PassRunner runner(module, parentRunner->options);
+  PassUtils::FilteredPassRunner runner(module, funcs);
+  runner.options = parentRunner->options;


It seems nicer to pass the options in to the pass runner constructor rather than imperatively setting them afterward. Can we add the options to the FilteredPassRunner constructor?

tlively · 2023-11-28T02:04:12Z

src/passes/pass-utils.h

+    return std::make_unique<FilteredPass>(pass->create(), relevantFuncs);
+  }
+
+  FilteredPass(std::unique_ptr<Pass> pass, const FuncSet& relevantFuncs)


This should be an rvalue reference so you don't have to move-construct the parameter just to move it again.

Suggested change

FilteredPass(std::unique_ptr<Pass> pass, const FuncSet& relevantFuncs)

FilteredPass(std::unique_ptr<Pass>&& pass, const FuncSet& relevantFuncs)

How does that work? I can make this change, but I cannot remove either of the std::moves in this file (use of deleted function errors etc.), so I'm not sure how it helps.

Before this change, the unique_ptr move constructor gets called twice. First, this pass parameter is move-constructed from whatever rvalue you pass into this FilteredPass constructor. Second, you move-construct the pass member from the pass parameter below in the initializer list with pass(std::move(pass)).

After this change, pass(std::move(pass)) would move-construct the pass member from the rvalue reference you pass into the FilteredPass constructor without move-constructing the pass parameter as a separate intermediate value in the middle.

I'm not sure if it makes any difference after optimizations, but in principle this change results in one less call to the unique_ptr move constructor.

Interesting, thanks.

I have very little idea of how the optimizer handles this stuff, but sounds like it can help. Added in the last push.

tlively · 2023-11-28T02:06:33Z

src/passes/pass-utils.h

+  }
+
+private:
+  std::unique_ptr<Pass> pass;


Instead of having an indirection here, it might make sense to use templates to store the pass inline in the FilteredPass<P> (or even as the supertype of FilteredPass<P>, which would avoid having to implemented modifiesBinaryenIR, etc.). WDYT?

I don't see how FilteredPass can be templated over the pass. The pass arrives from FilteredPassRunner::doAdd which does not have that information - it just gets a pass instance. (Templating doAdd might be possible but that would be a large change I think and I'm not sure if it can work or not.)

Ah right, if you don't statically know the full type at every callsite, templates won't work. Makes sense.

tlively · 2023-11-28T02:07:26Z

src/passes/pass-utils.h

+    : PassRunner(wasm), relevantFuncs(relevantFuncs) {}
+
+protected:
+  void doAdd(std::unique_ptr<Pass> pass) override {


I'm surprised doAdd is already virtual. Where else do we take advantage of this?

Nowhere, I think 😄 it was added for the code I am refactoring IIRC.

I think this is a good design because speed does not matter here: adding a pass is 1000x faster than running the typical pass. And it makes it simple to add such indirection.

Makes sense to me 👍

tlively · 2023-11-28T02:08:53Z

src/passes/pass-utils.h

+namespace wasm {
+
+namespace PassUtils {


Suggested change

namespace wasm {

namespace PassUtils {

namespace wasm::PassUtils {

kripken · 2023-11-30T00:08:03Z

It turns out that this is not exactly NFC, but actually has a benefit:

This avoids the hack of creating a module with only the functions we want to optimize, when we optimize after inlining. That is, before this PR we'd optimize a fake module containing only functions we inlined into (to avoid optimizing on code we did not change at all); after this PR we optimize the real module but use a filtering mechanism to only optimize in the functions we inlined.
Our effect analyzer can look at functions to see if they have effects: that is how the call.without.effects intrinsic works. We see that that function is an import, and identify it as the intrinsic from the module and base names.
In the hack we removed, the import was not in the module, since we removed all functions we didn't inline into. So we couldn't tell it was an intrinsic and assumed the worst. (We allow the function to simply not exist, as we may be optimizing before we finish building up all the IR.) After this PR, we identify it as the intrinsic and can see it has no side effects, so it can be removed.

That should not matter in the long term, as this just means that optimizations after inlining are now a tiny bit more effective than they were before; in particular the very next vacuum will remove those intrinsics. But this can accelerate optimization, that is, fewer rounds are needed to get the same results.

…bAssembly#6132) Asyncify gained a way to wrap a pass so that it only runs on a given set of functions, rather than on all functions, so the wrapper "filters" what the pass operates on. That was useful in Asyncify as we wanted to only do work on functions that Asyncify actually instrumented. There is another place in the code that needs such functionality, optimizeAfterInlining, which runs optimizations after we inline; again, we only want to optimize on the functions we know are relevant because they changed. To do that, move that logic out to a general place so it can be reused. This makes the code there a lot less hackish. While doing so make the logic only work on function-parallel passes. It never did anyhow, but now it asserts on that. (It can't run on a general pass because a general one does not provide an interface to affect which functions it operates on; a general pass is entirely opaque in that way.)

kripken added 10 commits November 22, 2023 15:40

start

8aa5b69

start

1c7ed9f

fix

1f727eb

fix

f917b08

work

04c158b

simpl

1b0cfd7

rename

52df392

rename

6690186

format

e6b2648

simpl

3b9b855

kripken commented Nov 27, 2023

View reviewed changes

kripken requested a review from tlively November 27, 2023 22:27

tlively reviewed Nov 28, 2023

View reviewed changes

kripken added 4 commits November 28, 2023 13:05

feedback

10fffc4

feedback

cbb4eeb

repeat feedback in another place

d5dfd0b

oops

f0bc13d

tlively approved these changes Nov 28, 2023

View reviewed changes

feedback: &&

6ba7c24

kripken enabled auto-merge (squash) November 28, 2023 23:11

kripken merged commit dbcac17 into main Nov 28, 2023
14 checks passed

kripken deleted the wrap.pass branch November 28, 2023 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NFC] Move InstrumentedPass logic out and use it in another place #6132

[NFC] Move InstrumentedPass logic out and use it in another place #6132

kripken commented Nov 23, 2023

kripken Nov 27, 2023

tlively Nov 28, 2023

tlively Nov 28, 2023

kripken Nov 28, 2023

tlively Nov 28, 2023

kripken Nov 28, 2023

tlively Nov 28, 2023

kripken Nov 28, 2023

tlively Nov 28, 2023

tlively Nov 28, 2023

kripken Nov 28, 2023

tlively Nov 28, 2023

tlively Nov 28, 2023

kripken commented Nov 30, 2023

	FilteredPass(std::unique_ptr<Pass> pass, const FuncSet& relevantFuncs)
	FilteredPass(std::unique_ptr<Pass>&& pass, const FuncSet& relevantFuncs)

[NFC] Move InstrumentedPass logic out and use it in another place #6132

[NFC] Move InstrumentedPass logic out and use it in another place #6132

Conversation

kripken commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken commented Nov 30, 2023