-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
improve asm copy propagation for MOVE instruction #6641
base: master
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #6641 will not alter performanceComparing Summary
|
63b0bfa
to
80d5a52
Compare
VirtualOp::ADD(dest, opd1, opd2) => { | ||
// We don't look for the first operand being a constant and the second | ||
// one a base register. Such patterns must be canonicalised prior. | ||
if let Some(&RegContents::Constant(c2)) = reg_contents.get(opd2) { |
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.
Am I correct in understanding that this is a bug fix? (unrelated to MOVs). We're missing an optimization opportunity here
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 should be the same code, but with a if let
to accommodate the fact that this runs inside a retain_mut
now.
}, | ||
VirtualOp::MOVE(dest, src) => { | ||
let ver = get_def_version(&latest_version, src); | ||
if let Some(RegContents::BaseOffset(src, 0)) = reg_contents.get(src) { |
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.
are you trying to catch something like
a = b
b = a
?? Not sure I understand what is it that you're trying to optimize, and why the existing copy propagation in the assembly generator doesn't apply.
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.
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.
On a = b
, this match arm does two things:
1 - first, it indeed checks if a
and b
are already the same, and it removes the instruction;
2 - but it also propagates that b
and a
are the same for future optimizations.
Description
Checklist
Breaking*
orNew Feature
labels where relevant.