Update exact reference semantics #7499
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Revert the following PRs that introduced exactness on HeapType: - #7446 - #7444 - #7432 - #7412 - #7396 Keep only the changes to wasm-type-printing.cpp to fix the assertions that subclasses of TypeNameGeneratorBase correctly override getNames. Although putting exactness on heap types makes the most sense in the Custom Descriptors spec, it is not a great fit for how HeapType is used in Binaryen. In almost all cases, HeapType is used to represent heap type definitions rather than the dynamic types of values. Letting HeapType represent exact heap types is not useful in those cases and opens up a new class of possible bugs. To avoid these bugs, we had been introducing a new HeapTypeDef type to represent heap type definitions, but nearly all uses of HeapType would have had to have been replaced with HeapTypeDef. Rather than introduce HeapTypeDef as a third type alongside Type and HeapType, it will be simpler to continue using HeapType to represent heap type definitions and Type to represent the dynamic types of values, including their exactness. While exactness will syntactically be part of the heap type, it will be part of the `Type` in Binaryen IR. A follow-on PR will restore the original work on exact references, and PRs following that one will update the encoding, parsing, and printing of exact references to match the current spec.
In preparation for disallowing inexact references to abstract heap types, which are no longer allowed by the Custom Descriptors proposal.
Disallow exact references to abstract heap types and update subtyping to match the current proposed semantics.
kripken
reviewed
Apr 15, 2025
| @@ -795,13 +795,11 @@ Type Type::getLeastUpperBound(Type a, Type b) { | |||
| if (auto heapType = HeapType::getLeastUpperBound(heapTypeA, heapTypeB)) { | |||
| auto nullability = | |||
| (a.isNullable() || b.isNullable()) ? Nullable : NonNullable; | |||
| auto exactness = (a.isInexact() || b.isInexact()) ? Inexact : Exact; | |||
| // The LUB can only be exact if the heap types are the same or one of them | |||
| // is bottom. | |||
There was a problem hiding this comment.
This comment still seems useful, perhaps modified?
|
|
||
| assertLUB(exactSup, exactSup, exactSup, exactSup); | ||
| assertLUB(exactSup, nullExactSup, nullExactSup, exactSup); | ||
| assertLUB(exactSup, sub, sup, none); |
There was a problem hiding this comment.
About the glb being none, how can I see that in the lattice diagram? I'm confused as to where exact types fit in a lattice like this:
any
|
super
|
sub
|
none
There was a problem hiding this comment.
any
| \
super .
| \
sub exact super
| \ \
. exact sub .
|
.
....
\||/
none
kripken
approved these changes
Apr 15, 2025
| @@ -1,6 +1,6 @@ | |||
| ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited. | |||
|
|
|||
| ;; RUN: foreach %s %t wasm-opt -all --preserve-type-order --string-lowering -S -o - | filecheck %s | |||
| ;; RUN: wasm-opt %s -all --preserve-type-order --string-lowering -S -o - | filecheck %s | |||
There was a problem hiding this comment.
Is there any harm to using foreach with a single module? it's future-proof...
There was a problem hiding this comment.
The only downside is that it makes it slightly more annoying to run the test manually, e.g. to repro bugs. Not much of a downside, but also a very easy change to make or undo, so I think it's worth it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Disallow exact references to abstract heap types and update subtyping
to match the current proposed semantics.