[llvm] 300997c - [AArch64] Don't rename registers with pseudo defs in Ld/St opt.

[Florian Hahn via llvm-commits]

Author: Florian Hahn
Date: 2020-01-22T09:26:25-08:00
New Revision: 300997c41a00b705ca10264c15910dd8d691ab75

URL: https://github.com/llvm/llvm-project/commit/300997c41a00b705ca10264c15910dd8d691ab75
DIFF: https://github.com/llvm/llvm-project/commit/300997c41a00b705ca10264c15910dd8d691ab75.diff

LOG: [AArch64] Don't rename registers with pseudo defs in Ld/St opt.

If the root def of for renaming is a noop-pseudo instruction like kill,
we would end up without a correct def for the renamed register, causing
miscompiles.

This patch conservatively bails out on any pseudo instruction.

This fixes https://bugs.chromium.org/p/chromium/issues/detail?id=1037912#c70

Added: 
    

Modified: 
    llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
    llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
index 3156bb446963..bc91d628f0b4 100644
--- a/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
+++ b/llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
@@ -1325,6 +1325,19 @@ canRenameUpToDef(MachineInstr &FirstMI, LiveRegUnits &UsedInBetween,
 
     // For defs, check if we can rename the first def of RegToRename.
     if (FoundDef) {
+      // For some pseudo instructions, we might not generate code in the end
+      // (e.g. KILL) and we would end up without a correct def for the rename
+      // register.
+      // TODO: This might be overly conservative and we could handle those cases
+      // in multiple ways:
+      //       1. Insert an extra copy, to materialize the def.
+      //       2. Skip pseudo-defs until we find an non-pseudo def.
+      if (MI.isPseudo()) {
+        LLVM_DEBUG(dbgs() << "  Cannot rename pseudo instruction " << MI
+                          << "\n");
+        return false;
+      }
+
       for (auto &MOP : MI.operands()) {
         if (!MOP.isReg() || !MOP.isDef() || MOP.isDebug() || !MOP.getReg() ||
             !TRI->regsOverlap(MOP.getReg(), RegToRename))

diff  --git a/llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir b/llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir
index 018827772da5..b57e32338f07 100644
--- a/llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir
+++ b/llvm/test/CodeGen/AArch64/stp-opt-with-renaming.mir
@@ -469,3 +469,36 @@ body:             |
     RET undef $lr
 
 ...
+# Make sure we do not rename if pseudo-defs. Noop pseudo instructions like KILL
+# may lead to a missing definition of the rename register.
+#
+# CHECK-LABEL: name: test14_pseudo
+# CHECK: bb.0:
+# CHECK-NEXT:    liveins: $w8, $fp, $w25
+# CHECK:         renamable $w8 = KILL killed renamable $w8, implicit-def $x8
+# CHECK-NEXT:    STURXi killed renamable $x8, $fp, -40 :: (store 8)
+# CHECK-NEXT:    $w8 = ORRWrs $wzr, killed $w25, 0, implicit-def $x8
+# CHECK-NEXT:    STURXi killed renamable $x8, $fp, -32 :: (store 8)
+# CHECK-NEXT:    RET undef $lr
+#
+name:            test14_pseudo
+alignment:       4
+tracksRegLiveness: true
+liveins:
+  - { reg: '$x0' }
+  - { reg: '$x1' }
+  - { reg: '$x8' }
+frameInfo:
+  maxAlignment:    1
+  maxCallFrameSize: 0
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+    liveins: $w8, $fp, $w25
+
+    renamable $w8 = KILL killed renamable $w8, implicit-def $x8
+    STURXi killed renamable $x8, $fp, -40 :: (store 8)
+    $w8 = ORRWrs $wzr, killed $w25, 0, implicit-def $x8
+    STURXi killed renamable $x8, $fp, -32 :: (store 8)
+    RET undef $lr
+...