Fix type updating for indirect call effects#8874
Conversation
stevenfontanella
force-pushed
the
fix-indirect-call-effects
branch
9 times, most recently
from
7b92d58 to
9d72e54
Compare
stevenfontanella
force-pushed
the
fix-indirect-call-effects
branch
from
9d72e54 to
cb25670
Compare
|
Running a test against calcworker to confirm that the binary size savings return. |
The savings are more modest than before (1187 bytes), since before #8833 we were overly optimistic. I also tried with the other potential fix that I mentioned -- which is less conservative -- and only saves 5 additional bytes. |
| static Type i64Pair({Type::i64, Type::i64}); | ||
| return i64Pair; | ||
| } | ||
| Type getI64Pair() { return Type({Type::i64, Type::i64}); } |
There was a problem hiding this comment.
Is this change relevant to this PR? And is it performance-neutral?
There was a problem hiding this comment.
Yes, I explained why I made this change in the PR description. Without this change I hit a use-after-free when running unit tests for this PR. I'm not sure about the performance impact but without this change the tests failed. Let me repro and share it here.
Another potential solution would be to add some hook to reset these when destroyAllTypesForTestingPurposesOnly is called.
There was a problem hiding this comment.
Ah, I forgot about this issue since the last review... sgtm
There was a problem hiding this comment.
I wonder if there are other places we stash static types that would hit the same problem... a quick grep shows that wasm.cpp has a couple places where it stashes the result of getI64Pair() in a static variable.
| } | ||
| } | ||
|
|
||
| std::unordered_map<std::string, std::shared_ptr<const EffectAnalyzer>> |
There was a problem hiding this comment.
Maybe add a comment here? Not clear to me what the goal of this function is at a high level.
| auto [it, inserted] = newTypeEffects.emplace(newType, *oldEffects); | ||
| if (!inserted) { | ||
| auto merged = std::make_shared<EffectAnalyzer>(*it->second); | ||
| merged->mergeIn(**oldEffects); | ||
| it->second = std::move(merged); | ||
| } |
There was a problem hiding this comment.
Why do we have to allocate a new EffectAnalyzer when we already have one for the new type? Can we not just merge the old effects into the existing effects for the new type? Then this could be simplified:
| auto [it, inserted] = newTypeEffects.emplace(newType, *oldEffects); | |
| if (!inserted) { | |
| auto merged = std::make_shared<EffectAnalyzer>(*it->second); | |
| merged->mergeIn(**oldEffects); | |
| it->second = std::move(merged); | |
| } | |
| newTypeEffects[newType]->mergeIn(**oldEffects); |
| if (pass->addsEffects()) { | ||
| // Indirect call effects are now under-approximating. Clear them to avoid | ||
| // incorrect optimizations. | ||
| wasm->indirectCallEffects.clear(); | ||
| } |
There was a problem hiding this comment.
Can we write a test for which this is necessary?
3 participants
Part of #8615. Fixes #8833 which was wrong. The earlier fix never touched
newTypeEffectsat all. It passed tests and fixed the breakage only incidentally by being maximally conservative and losing all indirect call effects in the case of a type update. Fix this logic and add unit tests to vet this code better.Drive-by fixes:
addsEffects()is true. Clearing it when recomputing global effects is necessary to remove stale entries for types that no longer exist after type rewriting (although it should make no difference to optimizations).getMutI8Arrayand similar functions referred to an index inglobalHeapTypeStore/globalTupleStore, which gets cleared in unit tests withdestroyAllTypesForTestingPurposesOnly(), causing the index to refer to garbage. Change these to not use static storage so that they're always in sync withglobalHeapTypeStore/globalTupleStore.