Fix inlining of `return_call*` #6448

tlively · 2024-03-28T01:14:28Z

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 tranforming 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.

tlively · 2024-03-28T01:14:42Z

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

kripken · 2024-03-28T02:32:53Z

I haven't had time to review in detail yet, but a quick comment: I think you need to also fix the interpreter on return_call at this time, as if we just fix Inlining then it will be out of sync with execution and the fuzzer will find errors.

tlively · 2024-03-28T02:53:25Z

Yes, working on it now :)

kripken · 2024-03-28T16:34:08Z

src/passes/Inlining.cpp

+        auto var = builder->addVar(getFunction(), sig.params[i]);
+        branchBlock->list.push_back(
+          builder->makeLocalSet(var, curr->operands[i]));
+        curr->operands[i] = builder->makeLocalGet(var, sig.params[i]);


This can perhaps use ir/localize.h. That would avoid putting things in locals unnecessarily (it will leave things with no effects in place). But if it's more work maybe not worth it.

I will look into it.

It looks like this would be slightly tricky to use. The result of ChildLocalizer::getReplacement may or may not contain the parent (in this case the call), but we need to know where the parent is so we can replace it with a branch. We could add a new getChildrenReplacment method that returns a block with only the sets, perhaps.

Just pushed this change, PTAL.

kripken · 2024-03-28T16:35:16Z

src/passes/Inlining.cpp

  }
+
+  void walk(Expression*& curr) {
+    PostWalker<Updater>::walk(curr);


Rather than these two lines, can just implement visitFunction.

Oh no, because it's not the entire function that gets walked, just the block containing the inlined body.

Ah, thanks, that's what I was missing...

kripken · 2024-03-28T16:38:16Z

src/passes/Inlining.cpp

+      return;
+    }
+
+    Block* body = curr->cast<Block>();


Why must the function body be a block?

This is the block we create to hold the inlined contents in the parent. We always create a block in case there are any returns in the inlined contents that would need to be transformed into branches out of that block.

Are you sure you are passing the block? The inliner is crashing on this example.

(module (import "env" "h" (func $h (result i32))) (func $g (param $x i32) (result i32) (if (result i32) (local.get $x) (then (return_call $h) ) (else (i32.const 17) ) ) ) (func (export "f") (param $x i32) (result i32) (call $g (local.get $x)) ) )

No, this comment was incorrect. I fixed it though, so the latest version of this branch should be good. I just tested this example locally and it worked fine.

Oh, the fix is in #6451.

Oops, good catch. I've moved it to the correct PR now.

kripken · 2024-03-29T19:59:26Z

src/passes/Inlining.cpp

+    // inlined body. The branch label will be set later when we create branch
+    // targets for the calls.
+    Block* childBlock = ChildLocalizer(curr, getFunction(), *module, options)
+                          .getChildrenReplacement();


This should avoid adding temp vars when there is no need - is there a test that shows that? (I seem to see locals created all the time but I probably just missed it.)

tlively · 2024-03-29T20:11:49Z

test/lit/passes/inlining_enable-tail-call.wast

 ;; CHECK-NEXT:   (block
+ ;; CHECK-NEXT:    (block $__return_call
+ ;; CHECK-NEXT:     (block
+ ;; CHECK-NEXT:      (br $__return_call)


The lack of local.set here demonstrates that we successfully skipped using a local for this parameter. The test output still has a local because of the next inlining iteration.

Got it, thanks. Yeah, that local.set has two causes in these tests now made this hard to see...

kripken

lgtm with fuzzing + landing at the same time as the other PR.

kripken · 2024-03-29T21:00:09Z

test/lit/passes/inlining_enable-tail-call.wast

 ;; CHECK-NEXT:   (block
+ ;; CHECK-NEXT:    (block $__return_call
+ ;; CHECK-NEXT:     (block
+ ;; CHECK-NEXT:      (br $__return_call)


Got it, thanks. Yeah, that local.set has two causes in these tests now made this hard to see...

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 tranforming 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.

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 March 28, 2024 01:14

kripken reviewed Mar 28, 2024

View reviewed changes

tlively mentioned this pull request Mar 28, 2024

Fix interpretation of return_call* #6451

Merged

kripken reviewed Mar 29, 2024

View reviewed changes

tlively commented Mar 29, 2024

View reviewed changes

kripken approved these changes Mar 29, 2024

View reviewed changes

This was referenced Apr 2, 2024

Handle return calls in wasm-ctor-eval #6464

Merged

Update effects.h to handle return calls correctly #6470

Merged

tlively added 3 commits April 4, 2024 09:13

use ChildLocalizer

7f9d258

update test

12b5f92

tlively force-pushed the return-call-inlining branch from 25537ae to 12b5f92 Compare April 4, 2024 18:12

tlively mentioned this pull request Apr 5, 2024

Handle return calls in CodeFolding #6474

Merged

tlively added 2 commits April 5, 2024 12:23

fix incorrect assumption that inlined expression is block

b434f76

tlively enabled auto-merge (squash) April 8, 2024 17:10

tlively merged commit ad09739 into main Apr 8, 2024

tlively deleted the return-call-inlining branch April 8, 2024 17:50

Fix inlining of return_call* #6448

Fix inlining of return_call* #6448

Uh oh!

Conversation

tlively commented Mar 28, 2024

Uh oh!

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

Uh oh!

Uh oh!

kripken commented Mar 28, 2024

Uh oh!

tlively commented Mar 28, 2024

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!

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!

kripken left a comment

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

Fix inlining of `return_call*` #6448

Fix inlining of `return_call*` #6448

tlively commented Mar 28, 2024 •

edited

Loading