Skip to content

[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

Merged
merged 2 commits into from
Nov 1, 2023
Merged

[analysis] Add a lattice for value types #6064

merged 2 commits into from
Nov 1, 2023

Conversation

tlively
Copy link
Member

@tlively tlively commented Oct 30, 2023

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.

Copy link
Member

@kripken kripken left a 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?


// 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 {
Copy link
Member

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.
@tlively
Copy link
Member Author

tlively commented Oct 31, 2023

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not i64, f64?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do 👍

@tlively
Copy link
Member Author

tlively commented Nov 1, 2023

Merge activity

@tlively tlively merged commit 74237bf into main Nov 1, 2023
@tlively tlively deleted the valtype-lattice branch November 1, 2023 17:15
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants