-
Notifications
You must be signed in to change notification settings - Fork 786
[analysis] Add a lattice for value types #6064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
3c29d25
to
2d99383
Compare
b4d82c2
to
1458a3a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valtype.h
lgtm but I'm not sure what else I should review of these files?
src/analysis/lattices/array.h
Outdated
|
||
// A lattice whose elements are N-tuples of elements of L. Also written as L^N. | ||
// N is supplied at compile time rather than run time like it is for Vector. | ||
template<Lattice L, size_t N> struct Array { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't this in a previous PR?
Add a lattice that is a thin wrapper around `wasm::Type` giving it the interface of a lattice. As usual, `Type::unreachable` is the bottom element, but unlike in the underlying API, we uniformly treat `Type::none` as the top type so that we have a proper lattice.
2d99383
to
33139e2
Compare
Yep, overnight merge job for the previous PRs has messed up the diff here. Just pushed rebased versions to fix it. |
break; | ||
case 3: | ||
type = Type::f32; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not i64, f64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think adding them (or v128, or other reference types) would add much, since their relationships to other types isn't any different than what is already covered here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. Comment might be worthwhile as it's an obvious question for a reader of the code to have I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do 👍
Add a lattice that is a thin wrapper around `wasm::Type` giving it the interface of a lattice. As usual, `Type::unreachable` is the bottom element, but unlike in the underlying API, we uniformly treat `Type::none` as the top type so that we have a proper lattice.
Add a lattice that is a thin wrapper around
wasm::Type
giving it the interfaceof a lattice. As usual,
Type::unreachable
is the bottom element, but unlike inthe underlying API, we uniformly treat
Type::none
as the top type so that wehave a proper lattice.