Skip to content

Conversation

@hstk30-hw
Copy link
Contributor

This maybe a bug which is introduced by commit 6749ae3, and has been present ever since.
In this case, OtherReg always overlaps with DstReg cause they from the Copy all.

@llvmbot
Copy link
Member

llvmbot commented Nov 19, 2025

@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-backend-webassembly
@llvm/pr-subscribers-backend-hexagon
@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-llvm-regalloc

Author: None (hstk30-hw)

Changes

This maybe a bug which is introduced by commit 6749ae3, and has been present ever since.
In this case, OtherReg always overlaps with DstReg cause they from the Copy all.


Full diff: https://github.com/llvm/llvm-project/pull/168661.diff

1 Files Affected:

  • (modified) llvm/lib/CodeGen/RegisterCoalescer.cpp (+1-1)
diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp
index 25c4375a73ce0..e624088a0964e 100644
--- a/llvm/lib/CodeGen/RegisterCoalescer.cpp
+++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp
@@ -4150,7 +4150,7 @@ bool RegisterCoalescer::applyTerminalRule(const MachineInstr &Copy) const {
       continue;
     Register OtherSrcReg, OtherReg;
     unsigned OtherSrcSubReg = 0, OtherSubReg = 0;
-    if (!isMoveInstr(*TRI, &Copy, OtherSrcReg, OtherReg, OtherSrcSubReg,
+    if (!isMoveInstr(*TRI, &MI, OtherSrcReg, OtherReg, OtherSrcSubReg,
                      OtherSubReg))
       return false;
     if (OtherReg == SrcReg)

@hstk30-hw
Copy link
Contributor Author

There are many testcases fail, so if this is right, then I will fix and update it.

@s-barannikov
Copy link
Contributor

The fix looks intuitively correct to me

Copy link
Collaborator

@qcolombet qcolombet left a comment

Choose a reason for hiding this comment

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

Great catch @hstk30-hw!
Do you have a test case that exercises this problem.

@hstk30-hw
Copy link
Contributor Author

Fix all failed testcase.
I also push a commit hstk30-hw@d86334e which compare the old testcases that not turn on terminal rule with default turn on.

@github-actions
Copy link

🐧 Linux x64 Test Results

  • 186428 tests passed
  • 4863 tests skipped

@hstk30-hw
Copy link
Contributor Author

Do you have a test case that exercises this problem.

Not for now :)

I see luke get a nice performance improvement on MiBench with a 17% reduction in dynamic instruction count.
Can you test again with this patch? @lukel97
#165961 (comment)

@arsenm
Copy link
Contributor

arsenm commented Nov 21, 2025

Do you have a test case that exercises this problem.

Not for now :)

I think @qcolombet meant a mir lit test which shows the change, not benchmark results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants