Skip to content

ISLE: simplify select/bitselect when both choices are the same#6141

Merged
jameysharp merged 1 commit intobytecodealliance:mainfrom
Kmeakin:select-self
Apr 11, 2023
Merged

ISLE: simplify select/bitselect when both choices are the same#6141
jameysharp merged 1 commit intobytecodealliance:mainfrom
Kmeakin:select-self

Conversation

@Kmeakin
Copy link
Contributor

@Kmeakin Kmeakin commented Apr 4, 2023

Adds select x x => x and bitselect x x => x rewrites

@Kmeakin Kmeakin requested a review from a team as a code owner April 4, 2023 17:39
@Kmeakin Kmeakin requested review from elliottt and removed request for a team April 4, 2023 17:39
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Apr 4, 2023
@jameysharp
Copy link
Contributor

I like this commit. (Note for other reviewers, this PR is based on #6140 so at the moment only ISLE: simplify select/bitself when both choices are the same is necessary to review here.)

I'd like to double-check with @cfallin that these new rules are correct, but I think they are. I don't think there's any subtlety to select or bitselect that would make this optimization invalid.

I think this particular optimization might even be legal for select_spectre_guard, although that definitely has subtlety and also I wouldn't expect such a rule to ever fire.

There's a typo in the commit message and PR description ("bitself"→"bitselect") but once #6140 lands and this PR is rebased, I think this is a fine change.

@cfallin
Copy link
Member

cfallin commented Apr 4, 2023

Yep, this looks right to me -- no hidden subtleties!

I agree that the Spectre-guard variant would also be eligible (and would not lose any speculative safety), but I'm hesitant for us to do that, at least for now; IMHO a better approach for optimizations that touch select_spectre_guard is probably to (i) make changes that affect it only in concert with updates or optimizations to bounds-checking as a whole (i.e., when looking at generated code and improving it), when we're explicitly thinking about these issues; and (ii) do it in a separate PR, with a detailed argument around its speculative safety, probably also adding documentation on the topic.

@Kmeakin Kmeakin changed the title ISLE: simplify select/bitself when both choices are the same ISLE: simplify select/bitselect when both choices are the same Apr 5, 2023
@elliottt elliottt removed their request for review April 5, 2023 15:38
Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

Thank you!

@jameysharp jameysharp enabled auto-merge April 11, 2023 22:36
@jameysharp jameysharp added this pull request to the merge queue Apr 11, 2023
Merged via the queue into bytecodealliance:main with commit c0166f7 Apr 11, 2023
@Kmeakin Kmeakin deleted the select-self branch April 11, 2023 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants