Improve handling of COPY instructions with identical value numbers

Authored by kparzysz on Tue, Jun 12, 3:40 PM.


Diff Detail

kparzysz created this revision.Tue, Jun 12, 3:40 PM
tpr accepted this revision.Wed, Jun 13, 2:24 AM
tpr added a subscriber: tpr.

Yes, that works for me thanks on my larger test case.


Did you mean to leave this comment from my change in?

This revision is now accepted and ready to land.Wed, Jun 13, 2:24 AM
kparzysz marked an inline comment as done.Wed, Jun 13, 5:48 AM
This revision was automatically updated to reflect the committed changes.

This testcase is exposing some preexisting problem in the coalescer. I have reverted this patch until I figure out what's wrong.

kparzysz reopened this revision.Thu, Jun 14, 1:13 PM
This revision is now accepted and ready to land.Thu, Jun 14, 1:13 PM
kparzysz updated this revision to Diff 151411.Thu, Jun 14, 1:15 PM

Properly extend liveness of a merged value when a redundant copy was erased.

nhaehnle removed a subscriber: nhaehnle.Fri, Jun 15, 3:43 AM
tpr added a comment.Fri, Jun 15, 4:18 AM

With this, I'm getting a different assert "CopyMI input register not live" in one test case. I'll narrow it down and check I can reproduce it on upstream LLVM with your fix (as opposed to our local branch with your fix).

tpr added a subscriber: dstuttard.Sat, Jun 16, 4:04 AM
tpr added a comment.Sat, Jun 16, 10:46 AM

I can fix that assert on my testcase with this addition to your fix:

--- a/lib/CodeGen/RegisterCoalescer.cpp
+++ b/lib/CodeGen/RegisterCoalescer.cpp
@@ -1653,6 +1653,10 @@ bool RegisterCoalescer::joinCopy(MachineInstr *CopyMI, bool &Again) {
     return false;  // Not coalescable.
+  if (CopyMI->getOpcode() == TargetOpcode::IMPLICIT_DEF) {
+    // Not coalesceable; eliminateUndefCopy turned it into implicit_def.
+    return false;
+  }

   // Coalesced copies are normally removed immediately, but transformations
   // like removeCopyByCommutingDef() can inadvertently create identity copies.

I will go and test that more thoroughly.

tpr added a comment.Sun, Jun 17, 9:41 AM

I'm getting another failure somewhere else in our test suite with that. I need to analyze it further.