Add PostAssemblyScript pass

[dcodeIO]

As mentioned in #2398 I decided to give a PostAssemblyScript pass a try, and this is what I have so far.

Essentially, what the --post-assemblyscript pass does is to do a cheap traversal over each function first, detecting the presence and remembering key patterns of ARC-style code. Only if it finds such code, it creates a local graph and computes assignment influences between locals, which is a bit simpler than what the full local graph does.

Afterwards, it performs the following checks for each retain it found while traversing the function:

    // For each retain, check that it
    //
    // * doesn't reach a return (otherwise unbalanced)
    // * doesn't retain an allocation (otherwise necessary)
    // * reaches at least one release
    // * reaches only releases with balanced retains
    //

For each retain that fulfills the criteria, the retain itself and all releases it reaches are eliminated, which is very likely to result in code that other passes can optimize into something trivial like

__release(__retain(__alloc(...)))
__release(__retain(...))
__release(aConst)
__retain(aConst)

which a second pass, --post-assemblyscript-finalize, knows how to deal with.

In its current form, the pass requires running --flatten --post-assemblyscript --ANY_OTHER --post-assemblyscript-finalize --vacuum. I would imagine that there is a better way of doing all this, like without a local graph with some sort of flow algorithm, but oh well. Interestingly, since loops are guaranteed to have balanced retains and releases no matter their execution count, it appears that no extra logic is needed to deal with these.

On the AssemblyScript side, there is this PR using a custom Binaryen build using the passes, which so far appears to keep all existing tests intact.

Please excuse my C++ skills, I'm still in the process of getting used to it. Let me know what needs to be improved :)

[kripken]

I'd suggest doing this as a single pass, instead of two. It can contain internal sub-passes, and can still do a "pre", then run some other passes, then run a "post" pass. See for example Asyncify (look for PassRunner instances to see where the main pass runs sub-passes).

Otherwise, I don't understand the big picture here enough to review it. Perhaps you can write out a few examples of the concrete patterns that are optimized? Assume I know nothing about ARC, which is true :)

[dcodeIO]

Thanks for taking a look :) Updated the comments to include examples and made unique descriptions for the two passes. There is also this test (output) on our end now that verifies expected behavior for various patterns.

Thought behind the two passes was that inserting the passes explicitly, assuming interference with other passes, has the advantage that we can fine-tune pass order on our end, like here, where we know that we only need to flatten once due to local-cse preserving flatness (and reflattening flattened code blows it up pretty quickly by doubling local count and such, making intermediate results hard to grasp).

Also, if I ever implement a converge option on our end, we don't have to run --post-assemblyscript again (unless inlining, hmm, interesting), potentially not needing to flatten, with --post-assemblyscript-finalize being sufficient.

[kripken]

Very nice work! :)

I don't fully understand the AS-specific logic, so I can't review whether this is right given the semantics of your compiler. But otherwise this looks mostly good, with some comments.

How about adding isRetain and isRelease methods, that check the call name, and also assert on the right number of args if so? It seems like that could shorten this. E.g. eliminateRetain would begin with assert(isRetain(*location)), and it could be used in other places too like testReachesEscape.

Please add tests, in test/passes/. The file name includes the passes to be run, by default.

I couldn't quite guess why this requires flat IR. Can't it run without that? I think you'd need to also handle release(retain(..)) direct pairs like that, but maybe not much more?

[sbc100]

Should we create a separate tool called wasm-assemblyscripte-finalize to match the existing wasm-emscripten-finalize tool? Or that a bad analogy?

[dcodeIO]

I couldn't quite guess why this requires flat IR. Can't it run without that? I think you'd need to also handle release(retain(..)) direct pairs like that, but maybe not much more?

One example where flattening is useful is when assigning an object to a target already retaining a reference to another object

a = something;
...
a = newValue;

with the second assignment yielding outputs like

a = (
  if ((temp1 = newValue) != (temp2 = a)) {
    temp1 = __retain(temp1),
    __release(temp2),
  },
  temp1
);

i.e. a (block (result i32) ...) being assigned to the local. Might well be that there is a more clever way to follow retains and releases around. LocalGraph just appeared convenient because it can reuse a lot of existing code.

Please add tests, in test/passes/. The file name includes the passes to be run, by default.

Sure, will do. Would it be sufficient to just copy the test mentioned earlier over when it's finished?

How about adding isRetain and isRelease methods, that check the call name, and also assert on the right number of args if so? It seems like that could shorten this. E.g. eliminateRetain would begin with assert(isRetain(*location)), and it could be used in other places too like testReachesEscape.

This ended up requiring a set of functions. Tried to explain why under the review comment above, as for the other comments.

[kripken]

Would it be sufficient to just copy the test mentioned earlier over when it's finished?

Yeah, can be pretty basic. I assume you'll do extensive testing in AS itself. But basic tests here can prevent future unrelated refactorings from breaking you.

[kripken]

Nice! Looks good to me basically. Please just add some simple tests and I think this is ready to land.

[dcodeIO]

Alright, ported the tests validating the underlying assumptions to wasts now. These are relatively basic and check one pattern or edge case at a time, but should be enough to avoid breakage.

[kripken]

Great!

[kripken]

What's a good title / commit text for the squashed commit @dcodeIO ?

[dcodeIO]

Perhaps

Add PostAssemblyScript pass
Adds the AssemblyScript-specific passes post-assemblyscript and post-assemblyscript-finalize, eliminating redundant ARC-style retain/release patterns conservatively emitted by the compiler.

Does that sound ok? :)

[kripken]

Great!