-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[X86] Fix typo: QWORD alignment is greater than or equal to 8, not greater than 8 #87819
Conversation
@llvm/pr-subscribers-backend-x86 Author: AtariDreams (AtariDreams) ChangesThis probably inhibited a lot of optimizations. EmitTargetCodeForMemcpy does not have this problem, luckily. Full diff: https://github.com/llvm/llvm-project/pull/87819.diff 1 Files Affected:
diff --git a/llvm/lib/Target/X86/X86SelectionDAGInfo.cpp b/llvm/lib/Target/X86/X86SelectionDAGInfo.cpp
index 7c630a2b0da080..bc5cb5934454ce 100644
--- a/llvm/lib/Target/X86/X86SelectionDAGInfo.cpp
+++ b/llvm/lib/Target/X86/X86SelectionDAGInfo.cpp
@@ -86,7 +86,7 @@ SDValue X86SelectionDAGInfo::EmitTargetCodeForMemset(
ValReg = X86::EAX;
Val = (Val << 8) | Val;
Val = (Val << 16) | Val;
- if (Subtarget.is64Bit() && Alignment > Align(8)) { // QWORD aligned
+ if (Subtarget.is64Bit() && Alignment > Align(4)) { // QWORD aligned
AVT = MVT::i64;
ValReg = X86::RAX;
Val = (Val << 32) | Val;
|
But no tests are affected? |
Align(8) is QWORD aligned, but this was checking to see f alignment was higher than that, which it is not. Another PR I am working on for optsize depends on this, but because it is an oversight or typo that affects the compiler, I figure I would make this into a separate PR. |
Your original description said "This probably inhibited a lot of optimizations.". That should be pretty easy to prove so why no test updates? |
Because I am writing those tests right now. |
Done! |
67bab24
to
e243d36
Compare
…eater than 8 Align(8) is QWORD aligned, but this was checking to see if alignment was greater than that, when it should have been checking for being greater than OR EQUAL to Align(8). This bug was introduced in commit 6a6af30 during the transition to the Align type.
@phoebewang ping? |
I don't have commit access by the way. |
/cherry-pick 8389b3b |
Error: Command failed due to missing milestone. |
…to 8, not greater than 8 (llvm#87819) Align(8) is QWORD aligned, but this was checking to see if alignment was greater than that, when it should have been checking for being greater than OR EQUAL to Align(8). This bug was introduced in llvm@6a6af30d433d7 during the transition to the Align type. (cherry picked from commit 8389b3b)
Align(8) is QWORD aligned, but this was checking to see if alignment was greater than that, when it should have been checking for being greater than OR EQUAL to Align(8).
This bug was introduced in 6a6af30d433d7 during the transition to the Align type.