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

Cranelift: Rewrite (x>>k)<<k into masking off the bottom k bits #5673

Merged
merged 2 commits into from
Jan 31, 2023

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Jan 31, 2023

Thanks to Souper for pointing this one out! cc @regehr (let me know if you aren't interested in being cc'd on these!)

@fitzgen fitzgen added cranelift:goal:optimize-speed Focus area: the speed of the code produced by Cranelift. souper Issues and PRs related to the Souper superoptimizer labels Jan 31, 2023
@fitzgen fitzgen requested a review from cfallin January 31, 2023 18:58
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Question about a constant below, but general shape looks good.

Also I'm wondering if we should start a new file of rules, perhaps bits.isle or logical.isle, for cases like this; algebraic.isle is becoming a bit of a catch-all beyond its original "algebraic simplifications" roots.

cranelift/codegen/src/opts/algebraic.isle Outdated Show resolved Hide resolved
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Jan 31, 2023
@fitzgen fitzgen enabled auto-merge (squash) January 31, 2023 20:21
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

If it's not too much to ask for, then either in this PR or a followup, it would be great to get some runtests that trigger this rewrite as well (maybe just the tests you've added, duplicated with a few concrete invocations); that adds some more objective reassurance on top of golden-CLIF-output tests :-)

@fitzgen fitzgen disabled auto-merge January 31, 2023 20:28
@fitzgen fitzgen enabled auto-merge (squash) January 31, 2023 20:39
@fitzgen fitzgen merged commit 253e28c into bytecodealliance:main Jan 31, 2023
@fitzgen fitzgen deleted the shift-right-left-to-mask branch January 31, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:goal:optimize-speed Focus area: the speed of the code produced by Cranelift. cranelift Issues related to the Cranelift code generator souper Issues and PRs related to the Souper superoptimizer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants