Update effects.h to handle return calls correctly #6470

tlively · 2024-04-03T23:41:35Z

As far as their surrounding code is concerned return calls are no different from
normal returns. It's only from a caller's perspective that a function containing
a return call also has the effects of the return-callee. To model this more
precisely in EffectAnalyzer, stash the effects of return-callees on the side and
only merge them in at the end when analyzing the effects of a full function
body.

tlively · 2024-04-03T23:41:52Z

Handle return calls in CodeFolding #6474
Update effects.h to handle return calls correctly #6470 👈
Handle return calls in wasm-ctor-eval #6464
Fix interpretation of return_call* #6451
Fix inlining of return_call* #6448
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @tlively and the rest of your teammates on Graphite

tlively · 2024-04-03T23:42:15Z

@kripken, any suggestions you have about how to easily test this would be much appreciated.

kripken · 2024-04-03T23:51:53Z

How does a non-exception effect get noticed here? That is, when we discussed this before we talked about stashing a thrown exception on the side, and not all the effects. This does more work to stash them all, and I'm not sure why yet. (I get that technically the call executes after the return, so effects happen "later" - but it seems simpler and sufficient to track only exceptions IIANM.)

For testing throwing, you need global effects, see test/lit/passes/global-effects.wast. E.g. the call to $nop is optimized away thanks to global effects there. For our purposes here you can do a call to a function whose body is a try-catch-all of a return_call, so the only effect is a throw. By not optimizing it away we prove we still see that effect. And for comparison if the return_call were a call we do optimize it away.

tlively · 2024-04-03T23:59:46Z

My hope is that more precisely analyzing return call effects would lead to more optimization opportunities, but maybe that's not the case if control flow transfers cannot be reordered with any other side effect?

kripken · 2024-04-04T00:15:54Z

Do you mean the specific meaning of transfersControlFlow in this class? Yes, I don't think we reorder those with anything else at all. In principle perhaps we could in some cases, but we'd need to track more info than effects.h does, likely a CFG I'd guess.

tlively · 2024-04-04T18:24:14Z

Ok, I implemented the simplified version that only stashes the throws effect in Simplify to only stash the throws effect. But intentionally making the analysis less precise didn't sit right with me, so then I implemented a nicer design for stashing all the effects in Stash all effects inline in the effect analyzer.

Even if it doesn't make a functional difference now, being more precise by stashing all the effects can only possibly produce better results in the face of future optimizations for which it might make a difference. It also means we won't have to redo any of this work in the future if/when we add separate suspends or accessesDynamicScope or other potential scope-based effects.

kripken · 2024-04-04T18:29:10Z

src/ir/effects.h

+  // code, but the effects of the return-called functions are visible to
+  // callers. Collect effects of return-called functions here and merge them in
+  // only when analyzing an entire function.
+  EffectSet returnCallEffects;


This may double the size of EffectAnalyzer. Making it a unique_ptr might be better?

Though even that is worse than a bool.

kripken · 2024-04-04T18:31:39Z

I see your point that stashing effects could in theory help in the future. But do you see a practical way that could end up happening? That's what I am finding hard to see.

From my perspective, a return call is like a call except with a slight reordering, which has the quirk of causing the call to happen outside of current scoping rules. But that is all. And we do not track a CFG here, so tracking where the branch goes to is beyond the scope of this class - for that reason I am skeptical it will ever benefit from the theoretical additional precision.

tlively · 2024-04-04T18:47:17Z

Yeah, I guess I'm convinced that stashing will only ever be useful for effects that interact with scopes like throwing. Ok, back to the single bool.

src/ir/effects.h

vouillon · 2024-04-04T21:14:08Z

src/ir/effects.h

+
+      parent.calls = true;
+      if (parent.features.hasExceptionHandling() &&
+          (parent.tryDepth == 0 || curr->isReturn)) {


Suggested change

(parent.tryDepth == 0 || curr->isReturn)) {

(parent.tryDepth == 0 && !curr->isReturn)) {

test/lit/passes/global-effects.wast

tlively · 2024-04-04T22:37:32Z

Fuzzer seems happy with this. 20k+ iterations passed (without the last commit, which I'm fuzzing now).

vouillon · 2024-04-05T09:36:14Z

Two other possible tests. It seems the code folding pass could be improved to deal better with tail calls. I had to add a return to make it work.

 (func $code-folding (param $x i32)
  (try
   (do
    (drop (i32.const 1))
    (drop (i32.const 1))
    (return_call $throw)
    (return)
   )
   (catch_all
    (drop (i32.const 1))
    (drop (i32.const 1))
    (return_call $throw)
    (return)
   )
  )
 )

 (func $simplify-locals (param $x i32)
  (local $y i32)
  (local.set $y
   (if (result i32)
    (local.get $x)
    (then
     (local.get $x)
    )
    (else
     (return_call $throw)
    )
   )
  )
  (try
   (do
    (drop
     (local.get $y)
    )
   )
   (catch_all)
  )
 )

tlively · 2024-04-05T19:07:15Z

Thanks for the suggestions, @vouillon!

As far as their surrounding code is concerned return calls are no different from normal returns. It's only from a caller's perspective that a function containing a return call also has the effects of the return-callee. To model this more precisely in EffectAnalyzer, stash the effects of return-callees on the side and only merge them in at the end when analyzing the effects of a full function body.

This reverts commit 40f26ee.

kripken

lgtm % comments

src/ir/effects.h

kripken · 2024-04-05T19:50:14Z

src/ir/effects.h

+        // target function throws and we know that will be caught anyhow, the
+        // same as the code below for the general path. We can always filter out
+        // throws for return calls because they are already more precisely
+        // captured by `hasReturnCallThrow`.


Suggested change

// captured by `hasReturnCallThrow`.

// captured by `branchesOut`. That is, return calls add`branchesOut` which

// represents the fact that we are branching to another place that may

// have more effects, which can include throwing or anything else that

// happens when the call executes.

kripken · 2024-04-05T19:51:39Z

src/ir/effects.h

-      if (parent.features.hasExceptionHandling() && parent.tryDepth == 0) {
+      // When EH is enabled, any call can throw. Skip this for return calls
+      // because the throw is already more precisely captured by
+      // `hasReturnCallThrow`.


Suggested change

// `hasReturnCallThrow`.

// `branchesOut`.

branchesOut isn't related to the throw effect, though. It's the fact that we set hasReturnCallThrow that ensures the throw is eventually reported correctly.

Correct, but the context here is removing the throws effect on the next line. It is only valid to remove the throws effect because branchesOut indicates "there is a branch from here to some other effects". Those other effects are hasReturnCallThrow, but those do not come into play normally - not unless you walk the entire function. So it's the combination of the two, but branchesOut is the more immediate thing that "replaces" the throws we are removing, it kind of points to where the effects are at.

Consider that we still apply branchesOut even if the callee does not throw, though.

I'll update the comment to mention both.

We do, yes. But in the return_call case that has the additional important meaning of "linking" us to the effects we are removing here. So it explains why the removal of throws is valid in that case. That is, branchesOut becomes even more significant for return calls given this optimization here, and if we some day refactor branchesOut we'd need to consider this place.

kripken · 2024-04-05T19:59:41Z

test/lit/passes/global-effects.wast

+      (catch_all)
+    )
+    ;; This cannot be optimized out at all.
+    (call $return-call-throw-and-catch)


Please test return_call_indirect/ref as well, so we have coverage for them adding the hasReturnCallThrow field. (IIANM the tests below test the other property, that they do not have throws_ set and so they can be optimized better.)

Added tests, but I'm not sure they test what you're looking for because the callees also have the calls effect, which is enough to inhibit optimizations on its own.

Yeah, I guess calls masks this. Still, those tests look useful - maybe some day we'll make calls more refined somehow, and these would become more important.

Co-authored-by: Alon Zakai <azakai@google.com>

When an evaluated export ends in a return call, continue evaluating the return-called function. This requires propagating the parameters, handling the case that the return-called function might be an import, and fixing up local indices in case the final function has different parameters than the original function. * Update effects.h to handle return calls correctly (#6470) As far as their surrounding code is concerned return calls are no different from normal returns. It's only from a caller's perspective that a function containing a return call also has the effects of the return-callee. To model this more precisely in EffectAnalyzer, stash the throw effect of return-callees on the side and only merge it in at the end when analyzing the effects of a full function body.

We previously interpreted return calls as calls followed by returns, but that is not correct both because it grows the size of the execution stack and because it runs the called functions in the wrong context, which can be observable in the case of exception handling. Update the interpreter to handle return calls correctly by adding a new `RETURN_CALL_FLOW` that behaves like a return, but carries the arguments and reference to the return-callee rather than normal return values. `callFunctionInternal` is updated to intercept this flow and call return-called functions in a loop until a function returns with some other kind of flow. Pull in the upstream spec tests return_call.wast, return_call_indirect.wast, and return_call_ref.wast with light editing so that we parse and validate them successfully. * Handle return calls in wasm-ctor-eval (#6464) When an evaluated export ends in a return call, continue evaluating the return-called function. This requires propagating the parameters, handling the case that the return-called function might be an import, and fixing up local indices in case the final function has different parameters than the original function. * Update effects.h to handle return calls correctly (#6470) As far as their surrounding code is concerned return calls are no different from normal returns. It's only from a caller's perspective that a function containing a return call also has the effects of the return-callee. To model this more precisely in EffectAnalyzer, stash the throw effect of return-callees on the side and only merge it in at the end when analyzing the effects of a full function body.

This is a combined commit covering multiple PRs fixing the handling of return calls in different areas. The PRs are all landed as a single commit to ensure internal consistency and avoid problems with bisection. Original PR descriptions follow: * Fix inlining of `return_call*` (#6448) Previously we transformed return calls in inlined function bodies into normal calls followed by branches out to the caller code. Similarly, when inlining a `return_call` callsite, we simply added a `return` after the body inlined at the callsite. These transformations would have been correct if the semantics of return calls were to call and then return, but they are not correct for the actual semantics of returning and then calling. The previous implementation is observably incorrect for return calls inside try blocks, where the previous implementation would run the inlined body within the try block, but the proper semantics would be to run the inlined body outside the try block. Fix the problem by transforming inlined return calls to branches followed by calls rather than as calls followed by branches. For the case of inlined return call callsites, insert branches out of the original body of the caller and inline the body of the callee as a sibling of the original caller body. For the other case of return calls appearing in inlined bodies, translate the return calls to branches out to calls inserted as siblings of the original inlined body. In both cases, it would have been convenient to use multivalue block return to send call parameters along the branches to the calls, but unfortunately in our IR that would have required tuple-typed scratch locals to unpack the tuple of operands at the call sites. It is simpler to just use locals to propagate the operands in the first place. * Fix interpretation of `return_call*` (#6451) We previously interpreted return calls as calls followed by returns, but that is not correct both because it grows the size of the execution stack and because it runs the called functions in the wrong context, which can be observable in the case of exception handling. Update the interpreter to handle return calls correctly by adding a new `RETURN_CALL_FLOW` that behaves like a return, but carries the arguments and reference to the return-callee rather than normal return values. `callFunctionInternal` is updated to intercept this flow and call return-called functions in a loop until a function returns with some other kind of flow. Pull in the upstream spec tests return_call.wast, return_call_indirect.wast, and return_call_ref.wast with light editing so that we parse and validate them successfully. * Handle return calls in wasm-ctor-eval (#6464) When an evaluated export ends in a return call, continue evaluating the return-called function. This requires propagating the parameters, handling the case that the return-called function might be an import, and fixing up local indices in case the final function has different parameters than the original function. * Update effects.h to handle return calls correctly (#6470) As far as their surrounding code is concerned return calls are no different from normal returns. It's only from a caller's perspective that a function containing a return call also has the effects of the return-callee. To model this more precisely in EffectAnalyzer, stash the throw effect of return-callees on the side and only merge it in at the end when analyzing the effects of a full function body.

tlively requested a review from kripken April 3, 2024 23:41

This was referenced Apr 3, 2024

Fix inlining of return_call* #6448

Merged

Fix interpretation of return_call* #6451

Merged

Handle return calls in wasm-ctor-eval #6464

Merged

kripken mentioned this pull request Apr 3, 2024

Fix effect analysis in presence of tail calls within try blocks #6463

Closed

tlively force-pushed the return-call-ctor-eval branch from 668f7ec to c6497ac Compare April 4, 2024 18:12

tlively force-pushed the return-call-effects branch from 503afcf to de817ec Compare April 4, 2024 18:12

kripken reviewed Apr 4, 2024

View reviewed changes

src/ir/effects.h Show resolved Hide resolved

vouillon reviewed Apr 4, 2024

View reviewed changes

tlively commented Apr 4, 2024

View reviewed changes

test/lit/passes/global-effects.wast Show resolved Hide resolved

tlively mentioned this pull request Apr 5, 2024

Handle return calls in CodeFolding #6474

Merged

tlively added 7 commits April 5, 2024 12:23

Simplify to only stash the throws effect

1f11af9

Stash all effects inline in the effect analyzer

31ee4f7

test

f0d7c06

Revert "Stash all effects inline in the effect analyzer"

ddaa8fa

This reverts commit 40f26ee.

more tests

473e7ca

add simplify-locals test

cfa08a2

improve comments

5ff3513

tlively force-pushed the return-call-ctor-eval branch from c6497ac to 36a6b21 Compare April 5, 2024 19:24

tlively force-pushed the return-call-effects branch from b4affd2 to 5ff3513 Compare April 5, 2024 19:24

kripken approved these changes Apr 5, 2024

View reviewed changes

tlively and others added 5 commits April 5, 2024 13:44

Apply suggestions from code review

ca0dfe5

Co-authored-by: Alon Zakai <azakai@google.com>

test indirect and ref return calls

1074d48

mention branchesOut in addition to hasReturnCallThrow

0cf1b70

format

c790efc

add comment detail

6089f9c

tlively merged commit 3db9147 into return-call-ctor-eval Apr 8, 2024

tlively deleted the return-call-effects branch April 8, 2024 17:02

	(parent.tryDepth == 0 \|\| curr->isReturn)) {
	(parent.tryDepth == 0 && !curr->isReturn)) {

-        // captured by `hasReturnCallThrow`.
+        // captured by `branchesOut`. That is, return calls add`branchesOut` which
+        // represents the fact that we are branching to another place that may
+        // have more effects, which can include throwing or anything else that
+        // happens when the call executes.

Update effects.h to handle return calls correctly #6470

Update effects.h to handle return calls correctly #6470

Uh oh!

Conversation

tlively commented Apr 3, 2024

Uh oh!

tlively commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlively commented Apr 3, 2024

Uh oh!

kripken commented Apr 3, 2024

Uh oh!

tlively commented Apr 3, 2024

Uh oh!

kripken commented Apr 4, 2024

Uh oh!

tlively commented Apr 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Apr 4, 2024

Uh oh!

tlively commented Apr 4, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tlively commented Apr 4, 2024

Uh oh!

vouillon commented Apr 5, 2024

Uh oh!

tlively commented Apr 5, 2024

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tlively commented Apr 3, 2024 •

edited

Loading