Improve handling of COPY instructions with identical value numbers
AcceptedPublic

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

Details

Diff Detail

Repository
rL LLVM
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.

lib/CodeGen/RegisterCoalescer.cpp
2462

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) {
     deleteInstr(CopyMI);
     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.