Add a utility for comparing and hashing rec group shapes #6808
Conversation
| std::unordered_map<HeapType, Index> typeIndices; | ||
| for (auto type : types) { | ||
| typeIndices.insert({type, typeIndices.size()}); | ||
| if (type.getRecGroupIndex() == 0) { |
There was a problem hiding this comment.
What does index 0 mean that is special? I looked at the definition of the function in wasm-type.h and don't see an explanation. In fact I'm not sure what the index means - is it some kind of global unique index?
There was a problem hiding this comment.
getRecGroupIndex gives the index of type within its rec group, so whenever it is zero, we know we are starting a new rec group.
There was a problem hiding this comment.
Thanks. Please also add a comment to wasm-types.h.
tlively
force-pushed
the
fix-type-fuzzer-shared
branch
from
a4472a7
to
977c7c4
Compare
| std::unordered_map<HeapType, Index> typeIndices; | ||
| for (auto type : types) { | ||
| typeIndices.insert({type, typeIndices.size()}); | ||
| if (type.getRecGroupIndex() == 0) { |
There was a problem hiding this comment.
Thanks. Please also add a comment to wasm-types.h.
| } | ||
| } else { | ||
| // Hash collisions are technically possible, but should be rare enough | ||
| // that we can consider them bugs if the fuzzer finds them. |
There was a problem hiding this comment.
I actually did find one in practice, but then I generated some random numbers to mix in for the different type kinds and I didn't see one after that.
This is very similar to the internal utilities for canonicalizing rec groups in the type system implementation, except that the new utility also supports ordered comparison of rec groups, and of course the new utility only uses the public type API. A follow-up PR will replace the internal implementation of rec group comparison and hashing in the type system with this one. Another follow-up PR will use this new utility in a type optimization.
| // and equality differentiate the top-level structure of each type in the | ||
| // sequence and the equality of referenced heap types that are not in the | ||
| // recursion group, but for references to types that are in the recursion group, | ||
| // it considers only the index of the referenced type within the group. That |
There was a problem hiding this comment.
If it considers the index in the group, wouldn't these end up different?
(rec
(type $A (sub (struct (field (ref $B)))))
(type $B (sub (struct)))
)
(rec
(type $B (sub (struct)))
(type $A (sub (struct (field (ref $B)))))
)There was a problem hiding this comment.
Yes indeed, although those rec groups are already different just by looking at their top-level shapes. It would consider these different as well, where the top-level shapes are all the same:
(rec
(type $A (struct (field (ref $A))))
(type $B (struct (field (ref $B))))
)
(rec
(type $A (struct (field (ref $A))))
(type $B (struct (field (ref $A))))
)
This precisely matches the spec's notion of rec group identity.
There was a problem hiding this comment.
Oh, thanks, that's what I was missing. So this is matching the spec behavior, and not defining another notion. I thought a broader notion might make sense when looking at permutations of rec groups but I may have misunderstood the connection of this PR to your other work.
There was a problem hiding this comment.
Yep, this PR is providing the tools for detecting when the spec would consider two rec groups to the be same so we can then take action to differentiate them.
|
I think the type fuzzer is sufficient for now. It gets great coverage very easily, whereas we would have to have very many unit tests to get full coverage. I also aim to use this new code in the type system implementation soon, where it will be exercised by all of our existing tests. |
This is very similar to the internal utilities for canonicalizing rec
groups in the type system implementation, except that the new utility
also supports ordered comparison of rec groups, and of course the new
utility only uses the public type API.
A follow-up PR will replace the internal implementation of rec group
comparison and hashing in the type system with this one.
Another follow-up PR will use this new utility in a type optimization.