Refactor expression runner so it can be used via the C and JS APIs

[dcodeIO]

Refactors most of the precompute pass's expression runner into its base class so it can also be used via the C and JS APIs. Also adds the option to populate the runner with known constant local and global values upfront, and remembers assigned intermediate values as well as traversing into functions if requested.

C-API:

• ExpressionRunnerFlagsDefault()
• ExpressionRunnerFlagsPreserveSideeffects()
• ExpressionRunnerFlagsTraverseCalls()
• ExpressionRunnerCreate(module, flags, maxDepth, maxLoopIterations)
• ExpressionRunnerSetLocalValue(runner, index, value)
• ExpressionRunnerSetGlobalValue(runner, name, value)
• ExpressionRunnerRunAndDispose(runner, expr)

JS-API:

• binaryen.ExpressionRunner.Flags.Default
• binaryen.ExpressionRunner.Flags.PreserveSideeffects
• binaryen.ExpressionRunner.Flags.TraverseCalls
• new binaryen.ExpressionRunner(module[, flags[, maxDepth[, maxLoopIterations]]])
• binaryen.ExpressionRunner#setLocalValue(index, valueExpr)
• binaryen.ExpressionRunner#setGlobalValue(name, valueExpr)
• binaryen.ExpressionRunner#runAndDispose(expr)

[dcodeIO]

Last commit adds a mechanism to support evaluating expressions like

runner.runAndDispose(
  module.i32.add(
    module.block(null, [
      module.local.set(0, module.i32.const(5)),
      module.local.get(0, binaryen.i32)
    ], binaryen.i32),
    module.i32.const(1)
  )
);

where a value is local.set and later picked up again by a local.get, which is a common pattern in our code. Appears that precompute tests are unaffected by this, but chances are that I overlooked something. For instance, at one point I'm returning Flow() to indicate no value, instead of a breaking Flow(NONSTANDALONE_FLOW), which I'm not certain about.

[dcodeIO]

This now also implements what I mentioned earlier, in that constant local and global values known beforehand can be set explicitly. NFCI for existing code using the runner, but will open up new possibilities in that a generator does not have to inline these values manually, but can instead use locals as long as their constant value is known, e.g. const in JS where sometimes the value is known to be a compile time constant, and sometimes is just meant to be readonly.

Another interesting feature would be to utilize a sub-runner to traverse into lightweight function calls, so something like clamp or max functions can be evaluated.

[dcodeIO]

Last commit now also traverses into (simple) functions, which has some minor but beneficial effects on existing tests. However, for this to work I had to add a trapIfInvalid flag to the base class since it would otherwise abort on some test cases like test/unit.wat, which seems odd and might indicate that there is something wrong either in my code or in general.

[dcodeIO]

Hmm, appears this leads to non-determinism now, depending on the order that functions become optimized in, so sometimes a function is simple enough to evaluate and sometimes it's not (yet) :(

[tlively]

Looks great! I just have a few questions and small suggestions, but otherwise LGTM.

[tlively]

I would like to see more explanation of what preserving side effects means. Where/how are the side effects preserved?

[dcodeIO]

This now reads

// Be very careful to preserve any side effects. For example, if we are
// intending to replace the expression with a constant afterwards, even if we
// can technically evaluate down to a constant, we still cannot replace the
// expression if it also sets a local, which must be preserved in this scenario
// so subsequent code keeps functioning.

[tlively]

Suggested change

	// might or might not have been optimized already to something we can traverse
	// successfully, in turn leading to non-deterministic behavior.
	// might be concurrently modified, leading to undefined behavior.

The problem here is not so much that we don't know what state the other function is in, but that it's state could be changing and inconsistent when we try to traverse it. It would also be good to mention how this flag interacts with the PreserveSideEffects flag, if at all.

[dcodeIO]

Used your suggested change and mentioned that traversing another function uses this runner's flags, which implies PreserveSideEffects.

[tlively]

Are the side effects of these expressions preserved even without the PreserveSideEffects flag?

[dcodeIO]

This now reads

// Sets a known local value to use. Order matters if expressions have side
// effects. For example, if the expression also sets a local, this side effect
// will also happen (not affected by any flags). Returns `true` if the
// expression actually evaluates to a constant.

[tlively]

Why is this a std::move? What if the same local is gotten twice?

[dcodeIO]

My understanding here is that creating the std::pair<const wasm::Index, wasm::Literals> will make a copy of the wasm::Literals value, so using a std::move here hints that we can move that volatile copy instead of copying twice and dumping one. Perfectly possible that I don't actually know what I'm doing. Please advise :)

[tlively]

Couldn't there be subsequent gets if this is a tee, too?

[dcodeIO]

Good catch, updated the code accordingly

[tlively]

Would it be more idiomatic for the JS API to turn this into null or something like that?

[dcodeIO]

Figured that one would typically test this as !expr anyway, just doing an overly precise check here for testing purposes. Would imagine that not mixing 0 and null has benefits for the JIT.

[dcodeIO]

An alternative here is to return the unmodified original expression. Would that be an improvement?

[tlively]

I think it would make these tests easier to understand if the JSON were not hardcoded with the raw numbers for id and type. Would it be possible to explicitly construct the expected expressions instead?

[dcodeIO]

Used the respective constants now and added a little assertDeepEqual to make it more easily readable.

[tlively]

Awesome 👍

[tlively]

If the runner fails, it seems like it would be useful to expose more information to the caller about why it failed. That way the user could choose to increase the depth or loop count, if applicable. What do you think?

[dcodeIO]

Hmm, good question. Seems like this might be a bit too much, considering how it complicates the API. For instance, on the AssemblyScript side I expect to always use a reasonable maxDepth (or none) and give up otherwise as there is no reason to make an exception using larger limits. Would have used that limit right away then.

[tlively]

It's unfortunate that this will only work correctly if there is at most one Runner created at a time, but it's probably not worth fixing urgently. Perhaps you could at least leave a TODO about it?

[dcodeIO]

Implemented something working, but the code for it turned out to be a bit unattractive since, other than expressions etc., runners can be deleted leading to undefined behavior in tracing. Added comments.

[kripken]

Is this ready to land, or still waiting for review from @tlively ?

[tlively]

I'll take a final look now.

[tlively]

Just two smallish nits, but I'd be happy to see this merged as-is and have those cleaned up in non-urgent follow-ups. @kripken feel free to merge if you'd like.

[tlively]

This should probably be static, too. You could get extra fancy by making both of these helpers static variables inside of noteExpressionRunner to limit their scope, but I'll leave that up to you. OTOH, it would probably be better to just say we don't support making more than max size_t expression runners and get rid of all this logic, especially since it is literally impossible to have than many expression runners recorded in expressionRunners.

[dcodeIO]

Mostly thinking in terms of a very long lived process using Binaryen, let's say where modules are being created as-a-service. While we can't store max size_t in the structure, we might at some point overflow, where the likely scenario is that this is just fine, yet guarding for not reusing something left over (i.e. from a module created and never disposed) seems like a good precaution to have. Unlikely that someone will do this with tracing enabled, ofc.

[tlively]

Aha, I had missed that the ExpressionRunners were removed from the expressionRunners map when they were destroyed 👍

[tlively]

Can we do var i here, too, or would that be unidiomatic or bad? Seeing the variable reused like this gives me the heebie jeebies.

[dcodeIO]

On it, will fix these two real quick :)

[tlively]

@dcodeIO How is this for a commit message? (This just copied from the opening description)

---

Refactors most of the precompute pass's expression runner into its base class so it can also be used via the C and JS APIs. Also adds the option to populate the runner with known constant local and global values upfront, and remembers assigned intermediate values as well as traversing into functions if requested.

C-API:
ExpressionRunnerFlagsDefault()
ExpressionRunnerFlagsPreserveSideeffects()
ExpressionRunnerFlagsTraverseCalls()
ExpressionRunnerCreate(module, flags, maxDepth, maxLoopIterations)
ExpressionRunnerSetLocalValue(runner, index, value)
ExpressionRunnerSetGlobalValue(runner, name, value)
ExpressionRunnerRunAndDispose(runner, expr)

JS-API:
binaryen.ExpressionRunner.Flags.Default
binaryen.ExpressionRunner.Flags.PreserveSideeffects
binaryen.ExpressionRunner.Flags.TraverseCalls
new binaryen.ExpressionRunner(module[, flags[, maxDepth[, maxLoopIterations]]])
binaryen.ExpressionRunner#setLocalValue(index, valueExpr)
binaryen.ExpressionRunner#setGlobalValue(name, valueExpr)
binaryen.ExpressionRunner#runAndDispose(expr)

[dcodeIO]

Looks good :) (have been trying for a while to keep the first post good for a commit message, sometimes divided by a horizontal line to indicate where additional comments start)

[tlively]

@dcodeIO Looks like there is a merge conflict to resolve now :(

[kripken]

Ok great, merging! Thanks for all the work here, and sorry it took this long, but sometimes more complex changes end up that way...

[aheejin]

I saw this now, and sorry for late questions. It looks this PR duplicates many of functionalities of RuntimeExpressionRunner in ExpressionRunner. Why is that? If Binaryen and C API want to make use of it, why can't they just use RuntimeExpressionRunner? I only took a brief look and I might well be mistaken, so please advise!

[dcodeIO]

Was under the impression that RuntimeExpressionRunner is a different beast and requires a lot of context, like memory and external interface, where what I wanted to achieve was to quickly evaluate an expression with limited context while the module is still being generated, for example to check if a static condition (i.e. in generics supporting varying types) is statically true or false, again affecting codegen. Some of these changes are also beneficial to the precompute pass (or previously lived in a separate PrecomputeExpressionRunner), which does something very similar.

[aheejin]

Thanks for the answer. Yes, I now think the functionalities duplicated are not in RuntimeExpressionRunner but more in PrecomputeExpressionRunner. I opened #2797 for this.