Skip to content

Use effects for indirect call expressions#8625

Open
stevenfontanella wants to merge 6 commits into
mainfrom
expression-effects
Open

Use effects for indirect call expressions#8625
stevenfontanella wants to merge 6 commits into
mainfrom
expression-effects

Conversation

@stevenfontanella
Copy link
Copy Markdown
Member

@stevenfontanella stevenfontanella commented Apr 19, 2026

Part of #8615. After #8609, we compute effects for indirect call expressions, but only reflect this in the call-site via the effects of the Function that contains the indirect call. That let us reason about effects only one layer of indirection away, for example in the following module:

(func $a
  (call_ref $t (...))
)

(func $b
  (call $a)
)

If we know that an indirect call to $t can't possibly have any effects (e.g. its only potential target is a nop), we'd be able to optimize away (call $a) but not the (call_ref) itself, since the effects only got stored in the effects of $a.

This PR lets us reason about indirect call effects at the expression level within function bodies by adding a map from HeapType to effects typeEffects in wasm::Module. As a result we can completely optimize out the call_ref in the above example.

Drive-by fixes:

  • Set an unconditional trap effect on call_indirect when the call type doesn't match the target table.
  • Correctly set branchesOut for return_call on call.without.effects. Previously this would not have a branchesOut effect which may have allowed incorrect reorderings (we shouldn't move an effectful expression above a return_call but we would have allowed this). Will follow up in return_call with call.without.effects optimizes incorrectly #8693.

Comment thread src/ir/effects.h Outdated
@stevenfontanella
Copy link
Copy Markdown
Member Author

Seems like JJ + Github don't play well when I'm on a branch based on another branch that had a merge commit. Will fix this after merging the other branch.

stevenfontanella added a commit that referenced this pull request Apr 24, 2026
When running in --closed-world, compute effects for indirect calls by
unioning the effects of all potential functions of that type. In
--closed-world, we assume that all references originate in our module,
so the only possible functions that we don't know about are imports.
Previously [we gave up on effects
analysis](https://github.com/WebAssembly/binaryen/blob/29b2d42e8a748fbe1095696d58a52b7bf83e2253/src/passes/GlobalEffects.cpp#L83-L87)
for indirect calls.

Yields a very small byte count reduction in calcworker (3799354 -
3799297 = 57 bytes). Also shows no significant difference in Binaryen
runtime: (0.1346069 -> 0.13375045 = <1% improvement, probably within
noise). We expect more benefits after we're able to share indirect call
effects with other passes, since currently they're only seen one layer
up for callers of functions that indirectly call functions (see the
newly-added tests for examples).

Followups:
* Share effect information per type with other passes besides just via
Function::effects (#8625)
* Exclude functions that don't have an address (i.e. functions that
aren't the target of ref.func) from effect analysis ()
* Compute effects more precisely for exact + nullable/non-nullable
references

Part of #8615.
Base automatically changed from indirect-effects-scc to main April 24, 2026 21:36
@stevenfontanella stevenfontanella force-pushed the expression-effects branch 3 times, most recently from 30a31e1 to 60665f3 Compare May 7, 2026 20:33
Gemini WIP

Try changing call effects
@stevenfontanella stevenfontanella marked this pull request as ready for review May 7, 2026 21:33
@stevenfontanella stevenfontanella requested a review from a team as a code owner May 7, 2026 21:33
@stevenfontanella stevenfontanella requested review from aheejin and removed request for a team May 7, 2026 21:33
Comment thread test/lit/passes/global-effects-closed-world-tnh.wast Outdated
Comment thread test/lit/passes/global-effects-closed-world.wast
Comment thread src/ir/effects.h Outdated
}
}

if (effects.throws_ && (parent.tryDepth > 0 || curr->isReturn)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do we use effect.throws() and when to use effect.throws_?
(Should we use throws() in all cases?)

Copy link
Copy Markdown
Member Author

@stevenfontanella stevenfontanella May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! throws() is throws_ || !delegateTargets.empty(). If we're delegated to and we catch, then it would be correct for throws() to be false, but it would remain true in our case because we can only clear throws_ (it wouldn't be right to just clear delegateTargets because it looks like it's used for other things too, even if the delegated exception gets caught). So we're overly pessimistic in this case.

This way preserves the existing logic (link) so I'd prefer to keep it as-is and fix it in a future PR. Let me try to repro a case where our effects are overly pessimistic and file an issue.

Copy link
Copy Markdown
Member

@aheejin aheejin May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe my memory of how this delegateTargets is working is patchy (even though it's probably me who added this..).

Good catch! throws() is throws_ || !delegateTargets.empty(). If we're delegated to and we catch, then it would be correct for throws() to be false

How can we at the same time be delegated to and catch? If we are a target of a delegate, that means we are try-delegate, not catch, no?

(it wouldn't be right to just clear delegateTargets because it looks like it's used for other things too, even if the delegated exception gets caught).

What are the other things it used for?

Anyway, keeping the existing logic as is in this PR and follow-up later sounds fine.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we at the same time be delegated to and catch?

I think this is the typical case for try-delegate? e.g. here I think the $outer_handler block is a delegateTarget and catches, so it should have no throw effect:

(func
  try $outer_handler
    try
      call $potentially_throwing_function
    delegate $outer_handler  ;; Forwards the exception to the outer try block
  catch
    call $handle_error
  end
)

OTOH we can also delegate to a block that doesn't catch at all, in which case that block needs to have a throw effect, e.g. here the anonymous block for the function body should have a throws effect:

(func
  try
    try
      call $some_throwing_function
    delegate 1  ;; Delegates to the caller, bypassing the outer try/catch
  catch
    ;; This catch block is completely ignored
  end
)

(I just read about this feature now FWIW, so I could be wrong)

What are the other things it used for?

I thought it was used in another pass but after looking again it's just shadowing the name. Still, just clearing delegateTargets seems wrong; even if an expression catches exceptions, it seems misleading to say that it wasn't delegated to.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, your understanding is correct; I was confused. We handle the "catching" effect in catch_all (because tryDepth> 0) but still conservative because we don't check tag matches for catch, also we don't check whether the try-delegate body itself throws or not. Given that catch_all greatly outnumbers catch at least in C++ (due to destructors), I doubt fixing the former would be a meaningful win, but maybe the latter can be a good missed opportunity. Not sure.

binaryen/src/ir/effects.h

Lines 565 to 586 in 36b7033

static void doStartCatch(InternalAnalyzer* self, Expression** currp) {
Try* curr = (*currp)->cast<Try>();
// This is conservative. When an inner try-delegate targets the current
// expression, even if the try-delegate's body can't throw, we consider
// the current expression can throw for simplicity, unless the current
// expression is not inside a try-catch_all. It is hard to figure out
// whether the original try-delegate's body throws or not at this point.
if (curr->name.is()) {
if (self->parent.delegateTargets.contains(curr->name) &&
self->parent.tryDepth == 0) {
self->parent.throws_ = true;
}
self->parent.delegateTargets.erase(curr->name);
}
// We only count 'try's with a 'catch_all' because instructions within a
// 'try' without a 'catch_all' can still throw outside of the try.
if (curr->hasCatchAll()) {
assert(self->parent.tryDepth > 0 && "try depth cannot be negative");
self->parent.tryDepth--;
}
self->parent.catchDepth++;
}

Anyway I'm not still sure in what context "clearing delegateTargets" popped up, but anyway, I don't see any reason we should do it when handling calls.

Comment thread src/ir/effects.h
Comment thread src/ir/effects.h Outdated
@stevenfontanella stevenfontanella requested a review from aheejin May 12, 2026 20:45
Comment thread src/ir/effects.h
Comment thread src/ir/effects.h
Comment thread src/wasm.h
// TODO: Use Type instead of HeapType to account for nullability and
// exactness.
std::unordered_map<HeapType, std::shared_ptr<const EffectAnalyzer>>
typeEffects;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this managed? Who is responsible for updating it? Please document this here.

Comment thread src/ir/effects.h
it != parent.module.typeEffects.end() && it->second) {
bodyEffects = it->second.get();
}
populateEffectsForCall(curr, bodyEffects);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps processCall or addCallEffects, which are shorter?

Comment thread src/ir/effects.h
Comment thread src/ir/effects.h
// GlobalEffects. Note that calls may have other effects that aren't
// captured by the function body of the target (e.g. a call_ref may trap on
// null refs).
template<typename CallType>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
template<typename CallType>
template<typename T>

I believe this is the pattern we use in general.

Comment thread src/support/utilities.h
using Ts::operator()...;
};

template<typename T> using NullablePtr = T;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

Comment thread src/ir/effects.h
template<typename CallType>
void populateFunctionBodyEffects(const CallType* curr,
const EffectAnalyzer& funcEffects) {
if (curr->isReturn) {
Copy link
Copy Markdown
Member

@kripken kripken May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

populateFunctionBodyEffects and populateEffectsForCall overlap here, I think? This is adding an effect from the call, not the body. Maybe I don't understand the difference between the functions - perhaps document them more?

Comment thread src/ir/effects.h
parent.trap = true;
return;
}
if (table->type.isNullable()) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we check only this but call trapOnNull on visitCallRef? Should we call trapOnNull on both cases?

Comment thread src/ir/effects.h
parent.throws_ = true;
const EffectAnalyzer* bodyEffects = nullptr;
if (auto it = parent.module.typeEffects.find(curr->heapType);
it != parent.module.typeEffects.end() && it->second) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a case it->second is null?

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.

4 participants