[PATCH] D44918: [RegisterCoalescing] Don't move COPY if it would interfere with another value

[Mikael Holmén via Phabricator via llvm-commits]

uabelho created this revision.
uabelho added reviewers: wmi, qcolombet.
Herald added subscribers: sdardis, MatzeB.

RegisterCoalescer::removePartialRedundancy tries to hoist B = A from
BB0/BB2 to BB1:

  BB1:
       ...
  BB0/BB2:  ----
       B = A;   |
       ...      |
       A = B;   |
         |-------
         |

It does so if a number of conditions are fulfilled. However, it failed
to check if B was used by any of the terminators in BB1. Since we must
insert B = A before the terminators (since it's not a terminator itself),
this means that we could erroneously insert a new definition of B before a
use of it.


https://reviews.llvm.org/D44918

Files:
  lib/CodeGen/RegisterCoalescer.cpp
  test/CodeGen/Mips/coalesce-partial-redundant-reguse-terminator.mir


Index: test/CodeGen/Mips/coalesce-partial-redundant-reguse-terminator.mir
===================================================================
--- /dev/null
+++ test/CodeGen/Mips/coalesce-partial-redundant-reguse-terminator.mir
@@ -0,0 +1,41 @@
+# RUN: llc -march=mips64 -o - %s -run-pass=simple-register-coalescing | FileCheck %s
+
+---
+name:            f
+tracksRegLiveness: true
+body:             |
+ bb.0:
+    successors: %bb.1
+
+    %21:gpr32 = ADDiu $zero, 0
+    %22:gpr32 = COPY %21
+    %22:gpr32 = ADDiu %22, 1
+    J %bb.1, implicit-def dead $at
+
+ bb.1:
+    successors: %bb.2
+
+    BEQ %22, $zero, %bb.2, implicit-def $at
+
+ bb.2:
+    successors: %bb.2, %bb.3
+
+    %22:gpr32 = COPY %21
+    %21:gpr32 = COPY %22
+    BEQ undef %0:gpr32, $zero, %bb.2, implicit-def $at
+
+ bb.3:
+
+...
+
+# We should not hoist the
+#
+#  %22:gpr32 = COPY %21
+#
+# into bb.1 since %22 is used in the BEQ.
+
+# CHECK-LABEL: bb.1:
+# CHECK-NOT:      COPY
+# CHECK:          BEQ
+
+# CHECK-LABEL: bb.2:
Index: lib/CodeGen/RegisterCoalescer.cpp
===================================================================
--- lib/CodeGen/RegisterCoalescer.cpp
+++ lib/CodeGen/RegisterCoalescer.cpp
@@ -989,13 +989,24 @@
   if (CopyLeftBB && CopyLeftBB->succ_size() > 1)
     return false;
 
-  // Now ok to move copy.
+  // Now (almost sure it's) ok to move copy.
   if (CopyLeftBB) {
+    // Position in CopyLeftBB where we should insert new copy.
+    auto InsPos = CopyLeftBB->getFirstTerminator();
+
+    // Make sure that B isn't referenced in the terminators (if any) at the end
+    // of the predecessor since we're about to insert a new definition of B
+    // before them.
+    if (InsPos != CopyLeftBB->end()) {
+      SlotIndex InsPosIdx = LIS->getInstructionIndex(*InsPos).getRegSlot(true);
+      if (IntB.overlaps(InsPosIdx, LIS->getMBBEndIdx(CopyLeftBB)))
+        return false;
+    }
+
     DEBUG(dbgs() << "\tremovePartialRedundancy: Move the copy to "
                  << printMBBReference(*CopyLeftBB) << '\t' << CopyMI);
 
     // Insert new copy to CopyLeftBB.
-    auto InsPos = CopyLeftBB->getFirstTerminator();
     MachineInstr *NewCopyMI = BuildMI(*CopyLeftBB, InsPos, CopyMI.getDebugLoc(),
                                       TII->get(TargetOpcode::COPY), IntB.reg)
                                   .addReg(IntA.reg);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D44918.139890.patch
Type: text/x-patch
Size: 2345 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180327/3ecaecff/attachment.bin>