Use indirect call effects in LinearExecutionWalker

[stevenfontanella]

Part of #8615. Follows up from #8637 now that global effects for indirect calls has been added.

Allows LinearExecutionWalker to not halt and continue scanning when an indirect call is encountered in the case the global effects have been computed under --closed-world and we determined that the indirect call can't have any throw effects. LinearExecutionWalker is used in the following passes:

• LocalGraph
• LocalCSE
• OptimizeCasts
• SimplifyGlobals
• SimplifyLocals
• GUFA
• TypeRefining

[tlively]

Nice!

[tlively]

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();

[stevenfontanella]

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.

[tlively]

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.

[stevenfontanella]

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.

[tlively]

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

Suggested change

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

[stevenfontanella]

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.

[tlively]

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

[tlively]

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.

[stevenfontanella]

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.

[tlively]

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

[tlively]

I would also fuzz this heavily, just in case.

[stevenfontanella]

I would also fuzz this heavily, just in case.

Ran overnight for 16k iterations with no issues.