Skip to content
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

[analysis] Simplify the stack lattice #6069

Merged
merged 1 commit into from
Nov 2, 2023
Merged

[analysis] Simplify the stack lattice #6069

merged 1 commit into from
Nov 2, 2023

Conversation

tlively
Copy link
Member

@tlively tlively commented Nov 1, 2023

Remove the ability to represent the top element of the stack lattice since it
isn't necessary. Also simplify the element type to be a simple vector, update
the lattice method implementations to be more consistent with implementations in
other lattices, and make the tests more consistent with the tests for other
lattices.

return NO_RELATION;
}
result = GREATER;
continue;
Copy link
Member

Choose a reason for hiding this comment

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

This switch sure looks familiar now... 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Maaaaybe it makes sense to try to share the switch logic. Note that everything surrounding the switch is slightly different in every case, though. (And in the tuple case, the switch contains recursive calls instead of continues.)

@kripken
Copy link
Member

kripken commented Nov 1, 2023

I only see one commit, so I'm not sure what changed since I approved this? Skimming it I don't see anything obviously different, and I didn't ask for changes?

@tlively
Copy link
Member Author

tlively commented Nov 1, 2023

The command I used to push and open #6069 must have re-requested review on this. Sorry for the noise!

@tlively tlively force-pushed the stack-lattice-fixups branch from 456aa3c to 350ba10 Compare November 2, 2023 17:02
@tlively
Copy link
Member Author

tlively commented Nov 2, 2023

Merge activity

  • Nov 2, 2:30 PM: @tlively started a stack merge that includes this pull request via Graphite.
  • Nov 2, 2:31 PM: Graphite rebased this pull request as part of a merge.
  • Nov 2, 2:31 PM: @tlively merged this pull request with Graphite.

Base automatically changed from shared-lattice to main November 2, 2023 18:30
Remove the ability to represent the top element of the stack lattice since it
isn't necessary. Also simplify the element type to be a simple vector, update
the lattice method implementations to be more consistent with implementations in
other lattices, and make the tests more consistent with the tests for other
lattices.
@tlively tlively force-pushed the stack-lattice-fixups branch from 350ba10 to 838bfac Compare November 2, 2023 18:31
@tlively tlively merged commit f191ace into main Nov 2, 2023
@tlively tlively deleted the stack-lattice-fixups branch November 2, 2023 18:31
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
Remove the ability to represent the top element of the stack lattice since it
isn't necessary. Also simplify the element type to be a simple vector, update
the lattice method implementations to be more consistent with implementations in
other lattices, and make the tests more consistent with the tests for other
lattices.
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