Skip to content

Use indirect call effects in LinearExecutionWalker #8738

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
stevenfontanella wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Use indirect call effects in LinearExecutionWalker #8738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc53f95
LinearExecutionWalker indirect call effects
stevenfontanella May 20, 2026
3cda588
PR updates
stevenfontanella May 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 40 additions & 21 deletions src/ir/linear-execution.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,12 @@ struct LinearExecutionWalker : public PostWalker<SubType, VisitorType> {
static void scan(SubType* self, Expression** currp) {
Expression* curr = *currp;

auto handleCall = [&](bool mayThrow, bool isReturn) {
auto handleCall = [&](bool isReturn, const EffectAnalyzer* effects) {
bool refutesThrowEffect = effects && !effects->throws_;
bool mayThrow = !self->getModule() ||
self->getModule()->features.hasExceptionHandling();
mayThrow = mayThrow && !refutesThrowEffect;

if (!self->connectAdjacentBlocks) {
// Control is nonlinear if we return or throw. Traps don't need to be
// taken into account since they don't break control flow in a way
Expand Down Expand Up @@ -156,40 +161,54 @@ struct LinearExecutionWalker : public PostWalker<SubType, VisitorType> {
case Expression::Id::CallId: {
auto* call = curr->cast<Call>();

bool mayThrow = !self->getModule() ||
self->getModule()->features.hasExceptionHandling();
if (mayThrow && self->getModule()) {
auto* effects =
self->getModule()->getFunction(call->target)->effects.get();

if (effects && !effects->throws_) {
mayThrow = false;
const EffectAnalyzer* effects = nullptr;
if (self->getModule()) {
auto* func = self->getModule()->getFunctionOrNull(call->target);
// TODO: `func` might not exist here because of #8753. Fix this
// and remove the null check.
if (func) {
effects = func->effects.get();
Comment on lines +166 to +170
Copy link
Copy Markdown
Member

@tlively tlively May 21, 2026

Choose a reason for hiding this comment

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

I would expect us to be able to assume that the target function exists, given that it would be invalid for it not to exist.

Suggested change
auto* func = self->getModule()->getFunctionOrNull(call->target);
if (func) {
effects = func->effects.get();
auto* func = self->getModule()->getFunction(call->target);
effects = func->effects.get();

Copy link
Copy Markdown
Member Author

@stevenfontanella stevenfontanella May 21, 2026

Choose a reason for hiding this comment

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

I thought so too, but when we run with --asyncify it causes us to fail when trying to lookup __asyncify_get_call_index. Maybe it's a problem with when the function is generated?

(lldb) f 1
frame #1: 0x00007ffff636e8a7 libbinaryen.so`wasm::LinearExecutionWalker<wasm::SimplifyLocals<true, false, true>, wasm::Visitor<wasm::SimplifyLocals<true, false, true>, void>>::scan(self=0x00007ffdc4009e50, currp=0x00007ffdb00019a8) at linear-execution.h:168:40
   165           if (self->getModule()) {
   166             auto* func = self->getModule()->getFunctionOrNull(call->target);
   167             if (true || func) {
-> 168               effects = func->effects.get();
   169             }
   170           }
   171
(lldb) p call->target
(wasm::Name) {
  wasm::IString = {
    str = (internal = "__asyncify_get_call_index")
  }
}

The repro is from test/lit/passes/asyncify_optimize-level=1.wast. The earlier code assumed that call targets exist but looks like it never hit an error before by luck.

Copy link
Copy Markdown
Member

@tlively tlively May 21, 2026

Choose a reason for hiding this comment

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

if (true || func) 😆 what is going on there?

It looks like Asyncify is temporarily adding calls to "intrinsics" that it later replaces with real code sequences. I think it should be fixed to add its "intrinsics" as actual function imports to avoid running other passes on invalid IR. But maybe that can be done as a follow-up. I'd be happy with a TODO here pointing to the problem.

Copy link
Copy Markdown
Member Author

@stevenfontanella stevenfontanella May 21, 2026

Choose a reason for hiding this comment

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

if (true || func) 😆 what is going on there?

I just quickly added that as an equivalent to your snippet to test out!

Sounds good, will add a todo here and follow up.

Copy link
Copy Markdown
Member Author

@stevenfontanella stevenfontanella May 22, 2026

Choose a reason for hiding this comment

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

Filed #8753 and added a TODO.

}
}

handleCall(mayThrow, call->isReturn);
handleCall(call->isReturn, effects);
break;
}
case Expression::Id::CallRefId: {
auto* callRef = curr->cast<CallRef>();

// TODO: Effect analysis for indirect calls isn't implemented yet.
// Assume any indirect call may throw for now.
bool mayThrow = !self->getModule() ||
self->getModule()->features.hasExceptionHandling();
const EffectAnalyzer* effects = [&]() -> const EffectAnalyzer* {
if (!self->getModule()) {
return nullptr;
}
if (!callRef->target->type.isRef()) {
Copy link
Copy Markdown
Member

@tlively tlively May 21, 2026

Choose a reason for hiding this comment

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

Might as well filter out nullfuncref and (ref nofunc) here.

Suggested change
if (!callRef->target->type.isRef()) {
if (!callRef->target->type.isSignature()) {

Copy link
Copy Markdown
Member Author

@stevenfontanella stevenfontanella May 21, 2026

Choose a reason for hiding this comment

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

I hadn't thought of that, but I think we don't want to do that because that means that we'd fail to lookup effects for calls to these types and be overly conservative as a result. We want the lookup to succeed and see empty effects which is what happens today:

binaryen/src/passes/GlobalEffects.cpp

Lines 176 to 178 in 84ace4a

// Add the key to ensure the lookup doesn't fail for indirect calls to
// uninhabited types.
callGraph[calleeType];
. Let me add a test for this case.

Copy link
Copy Markdown
Member

@tlively tlively May 21, 2026

Choose a reason for hiding this comment

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

I would expect these to get a trap effect and nothing else. Since the result is known, we shouldn't need a map lookup (unless it makes the code much simpler).

Copy link
Copy Markdown
Member Author

@stevenfontanella stevenfontanella May 22, 2026

Choose a reason for hiding this comment

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

The map will contain entries for those types with a trap effect:

binaryen/src/passes/GlobalEffects.cpp

Lines 317 to 323 in 505dae4

// This node came from an indirect call to an uninhabited type.
// This CC must consist of exactly one type, because an uninhabited type
// can't make any indirect calls to other types.
//
// Since the type is uninhabited, this call must trap.
assert(cc.size() == 1);
ccEffects->trap = true;
. Since the map contains the same information, I'm inclined to just look it up there instead of replicating the logic, although I guess it would be a small performance win? I don't like the idea of all users of indirectCallEffects having to replicate this logic.

Will add a test including an indirect call to (unreachable) to hit this code path.

return nullptr;
}

handleCall(mayThrow, callRef->isReturn);
auto* effectsPtr =
find_or_null(self->getModule()->indirectCallEffects,
callRef->target->type.getHeapType());
if (!effectsPtr) {
return nullptr;
}
return effectsPtr->get();
}();

handleCall(callRef->isReturn, effects);
break;
}
case Expression::Id::CallIndirectId: {
auto* callIndirect = curr->cast<CallIndirect>();

// TODO: Effect analysis for indirect calls isn't implemented yet.
// Assume any indirect call may throw for now.
bool mayThrow = !self->getModule() ||
self->getModule()->features.hasExceptionHandling();

handleCall(mayThrow, callIndirect->isReturn);
const EffectAnalyzer* effects = nullptr;
if (self->getModule()) {
if (const auto& effectsPtr =
find_or_null(self->getModule()->indirectCallEffects,
callIndirect->heapType)) {
effects = effectsPtr->get();
}
}
handleCall(callIndirect->isReturn, effects);
break;
}
case Expression::Id::TryId: {
Expand Down
8 changes: 8 additions & 0 deletions src/support/utilities.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,14 @@ template<typename... Ts> struct overloaded : Ts... {

template<typename... Ts> overloaded(Ts...) -> overloaded<Ts...>;

// Lookup a value from `map` and return a pointer to the underlying value
// or nullptr if not present. Returns a const pointer if `map` is const and
// non-const otherwise
auto* find_or_null(auto& map, const auto& key) {
Comment thread
stevenfontanella marked this conversation as resolved.
auto it = map.find(key);
return it != map.end() ? &it->second : nullptr;
}

} // namespace wasm

#endif // wasm_support_utilities_h
57 changes: 37 additions & 20 deletions test/lit/passes/simplify-locals-global-effects-eh.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,11 @@
)

(module
;; CHECK: (type $throw-type (func (result f64)))

;; CHECK: (type $const-type (func (result f32)))
(type $const-type (func (result f32)))

;; CHECK: (type $throw-type (func (result f64)))
(type $throw-type (func (result f64)))

;; CHECK: (global $g (mut i32) (i32.const 0))
Expand All @@ -68,7 +69,7 @@
;; CHECK: (table $t 2 2 funcref)
(table $t 2 2 funcref)

;; CHECK: (tag $t (type $2))
;; CHECK: (tag $t (type $3))
(tag $t)

;; CHECK: (func $const (type $const-type) (result f32)
Expand All @@ -90,32 +91,48 @@
)
(elem declare $throws)

;; CHECK: (func $read-g (type $3) (param $ref (ref null $const-type)) (result i32)
;; CHECK: (func $read-g-with-nop-call-ref (type $4) (param $ref (ref null $const-type)) (result i32)
;; CHECK-NEXT: (local $x i32)
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (global.get $g)
;; CHECK-NEXT: )
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (call_ref $const-type
;; CHECK-NEXT: (local.get $ref)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: (global.get $g)
;; CHECK-NEXT: )
(func $read-g (param $ref (ref null $const-type)) (result i32)
(func $read-g-with-nop-call-ref (param $ref (ref null $const-type)) (result i32)
(local $x i32)
(local.set $x (global.get $g))

;; With more precise effect analysis for indirect calls, we can determine
;; that the only possible target for this ref is $const in a closed world,
;; which wouldn't block our optimizations.
;; TODO: Add effects analysis for indirect calls.
;; With --closed-world enabled, we can tell that this can only possibly call
Copy link
Copy Markdown
Member

@tlively tlively May 21, 2026

Choose a reason for hiding this comment

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

I think --closed-world isn't even necessary for this, since this module has no imports or exports. There's no way for an outside function reference to sneak in.

Copy link
Copy Markdown
Member Author

@stevenfontanella stevenfontanella May 21, 2026

Choose a reason for hiding this comment

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

Good point, is it worth doing any indirect call analysis in the open world case? We could aggregate type effects unconditionally, and if we see a function ref that's imported or exported then invalidate the effects for that type and all subtypes. But any exported table of funcref would be enough to stop all analysis, and I know that many users use --closed-world anyway?

In practice, we only do indirect call analysis if --closed-world is enabled, so I think the comment is appropriate. It's still true that --closed-world is enough to determine that $const is the only possible callee here.

Copy link
Copy Markdown
Member

@tlively tlively May 21, 2026

Choose a reason for hiding this comment

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

I think we should do indirect call analysis in open world, too. It just needs to respect the public/private type classification it gets from module-utils.h. (In the long run we want everything to run in both open-world and closed-world and fully encapsulate the difference in the type visibility classfication.)

Copy link
Copy Markdown
Member Author

@stevenfontanella stevenfontanella May 22, 2026

Choose a reason for hiding this comment

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

I'll leave this alone since I think the comment is still accurate for now based on #8754. In the future we can change GlobalEffects to look at public/private for types instead of --closed-world at which point we can change this.

;; $const, which doesn't block our optimizations.
(drop (call_ref $const-type (local.get $ref)))

(local.get $x)
)

;; CHECK: (func $read-g-with-throw-in-between (type $4) (param $ref (ref $throw-type)) (result i32)
;; CHECK: (func $read-g-with-nop-call-indirect (type $5) (result i32)
;; CHECK-NEXT: (local $x i32)
;; CHECK-NEXT: (nop)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (call_indirect $t (type $const-type)
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (global.get $g)
;; CHECK-NEXT: )
(func $read-g-with-nop-call-indirect (result i32)
(local $x i32)
(local.set $x (global.get $g))

;; Similar to above with call_indirect instead of call_ref.
(drop (call_indirect (type $const-type) (i32.const 0)))

(local.get $x)
)

;; CHECK: (func $read-g-with-effectful-call-ref (type $2) (param $ref (ref $throw-type)) (result i32)
;; CHECK-NEXT: (local $x i32)
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (global.get $g)
Expand All @@ -127,7 +144,7 @@
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
(func $read-g-with-throw-in-between (param $ref (ref $throw-type)) (result i32)
(func $read-g-with-effectful-call-ref (param $ref (ref $throw-type)) (result i32)
(local $x i32)
(local.set $x (global.get $g))

Expand All @@ -138,25 +155,25 @@
(local.get $x)
)

;; CHECK: (func $read-g-with-call-indirect-in-between (type $5) (result i32)
;; CHECK: (func $read-g-with-effectful-call-indirect (type $2) (param $ref (ref $throw-type)) (result i32)
;; CHECK-NEXT: (local $x i32)
;; CHECK-NEXT: (local.set $x
;; CHECK-NEXT: (global.get $g)
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (call_indirect $t (type $const-type)
;; CHECK-NEXT: (call_indirect $t (type $throw-type)
;; CHECK-NEXT: (i32.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: (local.get $x)
;; CHECK-NEXT: )
(func $read-g-with-call-indirect-in-between (result i32)
(func $read-g-with-effectful-call-indirect (param $ref (ref $throw-type)) (result i32)
(local $x i32)
(local.set $x (global.get $g))

;; Similar to above with call_indirect instead of call_ref.
;; TODO: Add effects analysis for indirect calls.
(drop (call_indirect (type $const-type) (i32.const 0)))
;; Similar to above, except here we can tell that the indirect call may
;; throw so optimization is halted.
(drop (call_indirect (type $throw-type) (i32.const 0)))

(local.get $x)
)
Expand Down
Loading