Fix type updating for indirect call effects#8874
Conversation
7b92d58 to
9d72e54
Compare
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. |
| std::unordered_map<HeapType, std::shared_ptr<const EffectAnalyzer>> | ||
| newTypeEffects; | ||
|
|
||
| std::unordered_set<HeapType> unknownNewTypes; |
There was a problem hiding this comment.
A comment could clarify what "unknown" means in this variable name.
| find_or_null(wasm.indirectCallEffects, oldType); | ||
|
|
||
| if (!oldEffects) { | ||
| // oldType has no entry, which means its effects are explicitly unknown. |
There was a problem hiding this comment.
But the comment above says that if it is a new type, then not having an entry does not mean we must assume the worst? Perhaps I am not following what "explicitly unknown" means - can you clarify that in the comment?
There was a problem hiding this comment.
(I remember we discussed that term here in the PR, but (1) I don't remember the details 😄 and (2) it is good to define terms in the code, so it reads independently from the PR)
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.