Use effects for indirect call expressions

[stevenfontanella]

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.

[stevenfontanella]

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.

[aheejin]

This paragraph can maybe move to type-updating.cpp?

[stevenfontanella]

Alon suggested documenting it here: #8625 (comment)

There is a similar comment in type-updating.cpp:

  // Update indirect call effects per type.
  // When A is rewritten to B, B inherits the effects of A and A loses its
  // effects.

The part about individual functions being rewritten isn't relevant there since type-updating.cpp is for global type updates. Let me know if I should add anything else to make it more clear.

[kripken]

I don't understand the second paragraph here.

It sounds like it might be an optimization (we save an update we don't need) - if so, it doesn't need to be in the header. But it sounds like it also might be an invariant, in which case I am confused?

[stevenfontanella]

It's more a point about correctness than an optimization. If we computed effects for the type A, and then rewrote a function foo's type from A into something else, then it must be true that foo could never have been the target of an indirect call (otherwise this rewrite would be wrong), in which case (after #8644) its effects never would have been included under A anyway. And before #8644, our effects would just be out of date and overly conservative but not wrong.

OTOH if we rewrote a function's type from something else into A, then there's also no need to update anything, because any indirect call targeting A was anyway not targeting that function (even though it looks like it could after the rewrite).

Updated the comment to try to make it more clear.

[aheejin]

Can't we just blindly merge funcEffects here and deal with exception and tryDepth and isReturn stuff at the end of addCallEffects once and for all? (In this case we may not need addCallEffectsFromGlobalEffects as a separate function after all)

[stevenfontanella]

That's how the code was written before but I found it hard to follow with the two different cases interleaved. I prefer to expand it into the two cases where we have and don't have global effects. If you want to compare:

binaryen/src/ir/effects.h

Lines 718 to 771 in 2f1f55a

	void visitCall(Call* curr) {
	// call.without.effects has no effects.
	if (Intrinsics(parent.module).isCallWithoutEffects(curr)) {
	return;
	}
	// Get the target's effects, if they exist. Note that we must handle the
	// case of the function not yet existing (we may be executed in the middle
	// of a pass, which may have built up calls but not the targets of those
	// calls; in such a case, we do not find the targets and therefore assume
	// we know nothing about the effects, which is safe).
	const EffectAnalyzer* targetEffects = nullptr;
	if (auto* target = parent.module.getFunctionOrNull(curr->target)) {
	targetEffects = target->effects.get();
	}
	if (curr->isReturn) {
	parent.branchesOut = true;
	// When EH is enabled, any call can throw.
	if (parent.features.hasExceptionHandling() &&
	(!targetEffects || targetEffects->throws())) {
	parent.hasReturnCallThrow = true;
	}
	}
	if (targetEffects) {
	// We have effect information for this call target, and can just use
	// that. The one change we may want to make is to remove throws_, if the
	// target function throws and we know that will be caught anyhow, the
	// same as the code below for the general path. We can always filter out
	// throws for return calls because they are already more precisely
	// captured by `branchesOut`, which models the return, and
	// `hasReturnCallThrow`, which models the throw that will happen after
	// the return.
	if (targetEffects->throws_ && (parent.tryDepth > 0 || curr->isReturn)) {
	auto filteredEffects = *targetEffects;
	filteredEffects.throws_ = false;
	parent.mergeIn(filteredEffects);
	} else {
	// Just merge in all the effects.
	parent.mergeIn(*targetEffects);
	}
	return;
	}
	parent.calls = true;
	// When EH is enabled, any call can throw. Skip this for return calls
	// because the throw is already more precisely captured by the combination
	// of `hasReturnCallThrow` and `branchesOut`.
	if (parent.features.hasExceptionHandling() && parent.tryDepth == 0 &&
	!curr->isReturn) {
	parent.throws_ = true;
	}
	}

[stevenfontanella]

Will run the fuzzer for a few hours.

[stevenfontanella]

Ran 3900 iterations with no issues.

[kripken]

Why is this renamed from target to body? target seems natural given it is computed from curr->target etc.?

[stevenfontanella]

The reason is that bodyEffects represents the effects from running the function body, but not the effects from the call itself (e.g. branchesOut from return_call, trapping from a null pointer or type mismatch in call_indirect, etc.). I wanted to distinguish these two things and I feel that bodyEffects makes this more clear.

[kripken]

Related to the above, these effects may capture more than the function body. E.g. an import from JS may trap due to a type conversion of the parameters or results. (I don't think we compute effects for imports atm, but we could in the future, and probably should.)

[stevenfontanella]

Maybe I can change the comment to say Populate a call's effects using effects that were computed from GlobalEffects. i.e. "using" instead of "effects of the function's body". Does that sound more accurate?

Also, the effects from GlobalEffects really do only cover the function's body at the moment. If there are conversions at the JS boundary then those could be considered part of the body of a generated stub function (which IIUC is conceptually how this works).

Semi-related: are we able to compute effects for imports? I thought we wouldn't be able to determine anything about them unless they happen to be Binaryen intrinsics or string builtins. e.g. do we even know if a given import comes from JS or Wasm?

[kripken]

Suggested change

	auto filteredEffects = funcEffects;
	// We can ignore a throw here, as the parent catches it.
	auto filteredEffects = funcEffects;

Also need to explain the isReturn part. I actually don't know the reason for that off the top of my head?

[stevenfontanella]

Done, restored an earlier comment that explained it:

binaryen/src/ir/effects.h

Lines 747 to 751 in 7b79593

	// same as the code below for the general path. We can always filter out
	// throws for return calls because they are already more precisely
	// captured by `branchesOut`, which models the return, and
	// `hasReturnCallThrow`, which models the throw that will happen after
	// the return.

.

Also added a TODO based on the earlier conversation on this PR: #8625 (comment).

[kripken]

I don't understand the second paragraph here.

It sounds like it might be an optimization (we save an update we don't need) - if so, it doesn't need to be in the header. But it sounds like it also might be an invariant, in which case I am confused?

[stevenfontanella]

(Sorry for the force-push, I thought it would be fine to force-push changes to the latest commit that hasn't been looked at yet, but it seems that that still messed with the comment history)