Handle drops of unknown values in ExpressionRunner

[dcodeIO]

As suggested in #2786 this PR utilizes EffectAnalyzer on drops that failed to precompute to find out if all we were missing was a concrete local or global value, for example if an expression is being evaluated without full context.

fixes #2786

[dcodeIO]

Thinking about this a little more, dropping sets when not preserving side-effects is problematic since we might still be missing a nested set relevant to the final value. Seems this case must remain unsupported.

[aheejin]

I'm not sure if this is something the interpreter should do. Isn't this the area of optimization passes? I guess by using EffectAnalyzer we can do even more things, but I'm not sure if that should be functionalities of the interpreter in the first place. You can probably run wasm-opt and then run the interpreter to achieve the same effect.

[dcodeIO]

The use cases here are a bit blurry, in that precompute or scenarios like it benefit from it right away, while optimizations as a whole might not. This change in particular is required on my side to make the C-API's ExpressionRunner work with code like

if (a instanceof Foo) {
  ...
}

generating code like

if(
 (block (i32)
  (drop
   (local.get $a)
  )
  (i32.const 1)
 )
 (...)
)

where something may be computed statically. The condition can be arbitrarily complex. Like some earlier PRs, this is mostly about exposing Binaryen's toolkit to language developers in a friendly and useful way.

[aheejin]

I didn't review #2702 and I just noticed that Flow.breaking() returns true not only when we are actually breaking or returning, but also every time we can't precompute a value. I'm not sure if this is the design you intended, but it feels confusing..?

[dcodeIO]

Idea is that we do a cheap check on seenUnknownLocalGet first so we only spawn the analyzer if there is the chance that we succeed, but the flow might still be breaking to a label, which will never succeed. Except when breaking to NONCONSTANT_FLOW, which is the expected break target when seenUnknownLocalGet is true.

[aheejin]

What I meant is, I find Flow.breaking() returns true on non-breaking flows a little confusing. Before breaking() returns true only for control-flow-transferring expressions, such as branches or returns. But now it returns true too when it is set to NONCONSTANT_FLOW, which means every time you can't precompute a value.

[dcodeIO]

Yeah, this mechanism existed prior to my PRs, with two special break targets RETURN_FLOW and NONCONSTANT_FLOW (previously NONSTANDALONE_FLOW in precompute, then generalized and moved) indicating special conditions. My impression was that this is done so we don't need to throw exceptions, but can reuse breakTo? There is also a TODO on ExpressionRunner::trap to perhaps convert it to a break target eventually as well.

[aheejin]

The mechanism predates your PR was confined to PrecomputeExpressionRunner, and it handles it specially when checking Flow.breaking(). But now after #2702, all classes that inherits from ExpressionRunner treats NONCONSTANT_FLOW as breaking flow. It will return true every time we call Flow.breaking(), even if it's not breaking. And it is not practical to add checks like this every time we call Flow.breaking().

[dcodeIO]

From looking at the invocations of Flow.breaking(), these do either of the following:

• If the flow is breaking, maybe clean up and return the breaking flow, letting the caller deal with it or
• If the flow is breaking, check that breakTo is the expected label and error otherwise or
• If the flow is not breaking, do whatever

In all these cases whether breakTo is NONCONSTANT_FLOW is irrelevant (unless one wants to know that it is exactly that), quite similar to RETURN_FLOW, which is based on the same assumption and not confined. Or am I missing something?

[aheejin]

Hmm, I guess my main complaint is after all related to #2797. So I think Flow.breaking() should return true only when it is actually breaking, i.e., control flow transferring. But as you said, there was an exception of PrecomputeExpressionRunner, in which we had NOTSTANDALONE_FLOW or something. Now we effectively merged ExpressionRunner and PrecomputeExpressionRunner, the distinction is not clear, and suddenly we are treating nonbreaking flows as breaking. I agree this may not be a problem from the user perspective, but I find this pretty confusing now. Basically it feels like, we moved most of functionalities of PrecomputeExpressionRunner into ExpressionRunner base class even though many of them don't apply to other subclass RuntimeExpressionRunner, and RuntimeExpressionRunner is overriding all them. I guess we should continue this discussion in #2797.

[aheejin]

Thinking about this a little more, dropping sets when not preserving side-effects is problematic since we might still be missing a nested set relevant to the final value. Seems this case must remain unsupported.

Sorry I missed this comment. So are you still planning to land this?

[dcodeIO]

Yeah, I would still like to land this. Is useful even when bailing on local.sets for now (don't typically occur on my end and are irrelevant to the precompute pass), yet bailing on local.sets is somewhat half-baked. A proper variant would probably also spawn a LocalGraph when not preserving side-effects and the final value is a local.get to figure out if we can drop nonetheless, but this is getting quite a lot of code I fear. Would you prefer a comment explaining that more can be done, so we can revisit once that becomes relevant, or would you prefer that I look into supporting everything possible?

[aheejin]

Thinking about this a little more, dropping sets when not preserving side-effects is problematic since we might still be missing a nested set relevant to the final value. Seems this case must remain unsupported.

Could you provide an example of the case you explain here? I think it will help me understand.

Yeah, I would still like to land this. Is useful even when bailing on local.sets for now (don't typically occur on my end and are irrelevant to the precompute pass), yet bailing on local.sets is somewhat half-baked. A proper variant would probably also spawn a LocalGraph when not preserving side-effects and the final value is a local.get to figure out if we can drop nonetheless, but this is getting quite a lot of code I fear. Would you prefer a comment explaining that more can be done, so we can revisit once that becomes relevant, or would you prefer that I look into supporting everything possible?

I'm not sure if I understand what you mean by 'bailing on local.set'. An example will be appreciated here too 😅

[dcodeIO]

I initially attempted to support drops like

(drop
 (block (i32)
  (local.set $1 (i32.const 2))
  (local.get $0) ;; stops here if value is unknown
 )
)
(local.get $1) ;; correct value

but then figured that this can't work because execution is halted "somewhere", and there might still be sets we missed that are relevant, like

(local.set $1 (i32.const 1))
(drop
 (block (i32)
  (local.set $2
   (local.get $0) ;; stops here if value is unknown
  )
  (local.set $1 (i32.const 2)) ;; never executes
  (i32.const 3)
 )
)
(local.get $1) ;; wrong value

My assumption is that this can be supported by spawning a LocalGraph and backtracking the expression, just not sure how reasonable such an effort would be :)

[aheejin]

Hmm, yes, I think LocalGraph is too much; I wasn't even sure using EffectAnalyzer was the right thing in the interpreter. But you said you plan to land this; then how?

[dcodeIO]

It turned out that not covering the local.set case, but just local.get which can be done with EffectAnalyzer alone, is still good enough for my use case, so I'd be ok with this landing the way it is with a comment explaining that there's more that can be done. Hence my question whether you'd prefer that I instead look into this more, but might end up using LocalGraph?

Also, I meanwhile implemented a workaround functioning very much like this PR on our side, hence this isn't super urgent and we can take the time we need. The workaround is not quite as good as supporting it directly within the runner (still need to drop some expressions that are then missing from output before optimizations), but it enables me to at least use the ExpressionRunner today and scrap our previous precompute hack :)

[aheejin]

Sorry, maybe I'm missing your point, but

(local.set $1 (i32.const 1))
(drop
 (block (i32)
  (local.set $2
   (local.get $0) ;; stops here if value is unknown
  )
  (local.set $1 (i32.const 2)) ;; never executes
  (i32.const 3)
 )
)
(local.get $1) ;; wrong value

In this code you pasted above, you said this PR will return a wrong value. Then isn't landing this dangerous? You said for your usage this will be enough, but there's no guarantee other people will not use this with the code like above, no?

[dcodeIO]

Then isn't landing this dangerous?

No, since I reverted my initial attempt to deal with local.sets to avoid exactly that. This way around the PR only deals with drops of local.gets where the value is unknown, which is always safe to do.

[aheejin]

I see. Sorry for the delay. I'm still not sure about the use of EffectAnalyzer in the interpreter, but for this restricted use case I don't think there are a lot of risks either, so I'm torn.. WDYT @kripken?

[dcodeIO]

Perhaps a few thoughts: If we'd ultimately decide that dealing with such cases in ConstantExpressionRunner isn't worth it in general, that'd also mean that the usefulness of its new C-API is quite limited and we should probably revert it again. Even though unfortunate due to being back at the start, I'm not necessarily opposed to that, if it turns out that there's a better alternative - like something relying exclusively on passes.

[aheejin]

Hmm, I thought this was a small addition to the C API that has already landed. Are you planning to add many more PRs like this to make the C API useful? If not reverting the whole C API because of this sounds a little weird.. Is the C API in the current state (the things you landed so far) not very useful?

[dcodeIO]

Is the C API in the current state (the things you landed so far) not very useful?

API-wise it is much better than my previous hack. In general I believe that it is a really useful and powerful API for language developers, except that it suffers from the problem I tried to tackle here: Unlike in other parts of Binaryen, the value of a local can be "unknown" in the runner due to looking at an expression without context, so it gives up too early (on drops involving an unknown local value) even though it can technically still evaluate down to a value, which my use case demands (currently using a suboptimal workaround that modifies expressions even though it shouldn't). Quite unfortunate.

Are you planning to add many more PRs like this to make the C API useful?

No, dealing with drops involving unknown local values appears to be the only real bummer.

[aheejin]

I see, I guess this should be fine, and also this seems to help Precompute pass too. Sorry for the delays.

[aheejin]

Could you elaborate on what this means? Why do globals cause error when module is given?

[dcodeIO]

Good catch. Looked at this again and my assumption there is indeed wrong. I believe I based this on visitGlobalGet using Module::getGlobal, which asserts that the global exists, but I missed that the global may exist but not have a known value. Going to fix :)

[kripken]

Sorry I didn't read this earlier, but reading it now, I think I am even more on the side of my comments in #2786 - we should try to add useful general capabilities (like cloning to the C API, precompute without replacement, etc.) instead of adding specific optimizations like these, which should be doable using those general capabilities (clone some IR, precompute on it, see what happens to locals there, etc.).

[dcodeIO]

So, fwiw, I decided to address the earlier comments before starting over again. Planning to give the alternative approach a try, but figured it might makes sense to merge this one until there's something better available? Let me know what you think :)

[kripken]

Hmm, sorry, I still feel this is not quite the right approach, as vacuum should be capable of removing such drops. Can't we run that in the place you run precompute?

[dcodeIO]

Yeah, definitely not perfect. Can try to incorporate vacuum (into ExpressionRunner, or as a C-API) instead, but for that I'd need to clone the expression so the original doesn't become modified. What'd be the correct C++ API to clone an expression? Edit: ExpressionManipulator::copy?

[dcodeIO]

Gave copying a try in #2840, but ran into a problem with API tracing there :(

[kripken]

What'd be the correct C++ API to clone an expression? Edit: ExpressionManipulator::copy?

Yes, exactly, copy().