Skip to content

Conversation

@vouillon
Copy link
Contributor

@vouillon vouillon commented Apr 2, 2024

This is obviously correct: this accounts for the fact that tail calls can throw through exception handlers.

It is probably not worthwhile to make a more precise analysis:

  • The change only affects code with a tail call within a try block, which should be rare
  • Tail calls already have a branchesOut effect, which greatly limits possible optimizations

vouillon added 2 commits April 2, 2024 10:01
This is obviously correct: this accounts for the fact that tail calls
can throw through exception handlers.

It is probably not worthwhile to make a more precise analysis:
- The change only affects code with a tail call within a try block,
  which should be rare
- Tail calls already have a branchesOut effect, which greatly limits
  possible optimizations
@kripken
Copy link
Member

kripken commented Apr 2, 2024

Another option we discussed before was something like this:

  1. Track a new field on EffectAnalyzer, hasThrowingReturnCall.
  2. Set hasThrowingReturnCall in any return call, except for a direct call to a target we know cannot throw.
  3. In walk(Function* func) - the walk for an entire function body - if at the end we have hasThrowingReturnCall then set throws_.

That seems like it would be as precise as we can get it, but as you said, it might not be worth the overhead. Maybe it's worth mentioning as a possible future TODO though. I'm curious what other people think here.

@tlively
Copy link
Member

tlively commented Apr 2, 2024

I prefer the approach @kripken sketched because any local optimizations around the return call should be able to rely on the fact that it will not throw, at least not in a way that is observable by surrounding code.

@vouillon
Copy link
Contributor Author

vouillon commented Apr 3, 2024

To get a sense of how many optimizations we are missing, I have run wasm-opt -O3 on a 30 MiB binary with the following distribution of calls:

   1680 return_call_ref
   4316 call_ref
   4553 return_call
  17289 call

First with this PR, and a second time after modifying the code to (wrongly) assume that tail calls never throw:

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

The output code was exactly the same in both cases.

@vouillon
Copy link
Contributor Author

vouillon commented Apr 3, 2024

The output code was exactly the same in both cases.

Thinking a bit more about it, this is not that surprising. The only optimizations for which this makes a difference are the ones that eliminates a try block whose body does not throw (vacuum) or that moves code into (simplify-locals) or out of (code-folding) a try block.

Since my code does not contain any tail-call inside a try block, only the simplify-locals pass might make a difference. But the code has to be a bit convoluted for this to happen.

@kripken
Copy link
Member

kripken commented Apr 3, 2024

@vouillon to avoid duplicate effort, note that #6470 implements the other idea.

@vouillon
Copy link
Contributor Author

Superseded by #6470

@vouillon vouillon closed this Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants