-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
x64: Remove unnecessary register use when comparing against constants #4645
x64: Remove unnecessary register use when comparing against constants #4645
Conversation
;; As a special case, reverse the arguments to the comparison when the LHS is a | ||
;; constant. This ensures that we avoid moving the constant into a register when | ||
;; performing the comparison. | ||
(rule 1 (emit_cmp cc (and (simm32_from_value a) (value_type ty)) b) |
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'm not super happy with the need for a rule priority here, and would have expected the simm32_from_value
use to render it unnecessary. Any idea why it doesn't fire without higher priority?
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.
This can go away once #4661 is merged.
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
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.
This looks good to me as-is, but I'm curious to see if the rule-priority tweak we talked about today removes the need for the priority here before we merge it...
The bugfix you linked removes the need for the explicit priority here, so I'll make a separate PR for that before merging this without the priority 👍 |
bb4bbfb
to
8bb3209
Compare
8bb3209
to
8f6762b
Compare
8f6762b
to
dfc12a0
Compare
When lowering instructions like
icmp eq (iconst 0), v2
, we currently store the constant0
in a register before performing the comparison. The result is that for inputs like the following:we generate this assembly:
This PR introduces a case into
emit_cmp
that handles constants on the left-hand side of a comparison by swapping the order and reversing the operation. With this new rule, we emit the following assembly for the clif snippet above: