From: Geoff Berry <gberry@codeaurora.org>
Date: Mon, 28 Aug 2017 19:03:45 +0000 (+0000)
Subject: [ARM] Fix bug in ARMLoadStoreOptimizer when kill flags are missing.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=363f0068ddd28fe2c829d9a20629da014e855691;p=llvm

[ARM] Fix bug in ARMLoadStoreOptimizer when kill flags are missing.

Summary:
ARMLoadStoreOpt::FixInvalidRegPairOp() was only checking if one of the
load destination registers to be split overlapped with the base register
if the base register was marked as killed.  Since kill flags may not
always be present, this can lead to incorrect code.

This bug was exposed by my MachineCopyPropagation change D30751 breaking
the sanitizer-x86_64-linux-android buildbot.

Also clean up some dead code and add an assert that a register offset is
never encountered by this code, since it does not handle them correctly.

Reviewers: MatzeB, qcolombet, t.p.northover

Subscribers: aemerson, javed.absar, kristof.beyls, mcrosier, llvm-commits

Differential Revision: https://reviews.llvm.org/D37164

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@311907 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/lib/Target/ARM/ARMLoadStoreOptimizer.cpp b/lib/Target/ARM/ARMLoadStoreOptimizer.cpp
index 7a452d4a209..7f79fdca314 100644
--- a/lib/Target/ARM/ARMLoadStoreOptimizer.cpp
+++ b/lib/Target/ARM/ARMLoadStoreOptimizer.cpp
@@ -1559,12 +1559,10 @@ static bool isMemoryOp(const MachineInstr &MI) {
 
 static void InsertLDR_STR(MachineBasicBlock &MBB,
                           MachineBasicBlock::iterator &MBBI, int Offset,
-                          bool isDef, const DebugLoc &DL, unsigned NewOpc,
-                          unsigned Reg, bool RegDeadKill, bool RegUndef,
-                          unsigned BaseReg, bool BaseKill, bool BaseUndef,
-                          bool OffKill, bool OffUndef, ARMCC::CondCodes Pred,
-                          unsigned PredReg, const TargetInstrInfo *TII,
-                          bool isT2) {
+                          bool isDef, unsigned NewOpc, unsigned Reg,
+                          bool RegDeadKill, bool RegUndef, unsigned BaseReg,
+                          bool BaseKill, bool BaseUndef, ARMCC::CondCodes Pred,
+                          unsigned PredReg, const TargetInstrInfo *TII) {
   if (isDef) {
     MachineInstrBuilder MIB = BuildMI(MBB, MBBI, MBBI->getDebugLoc(),
                                       TII->get(NewOpc))
@@ -1584,6 +1582,8 @@ bool ARMLoadStoreOpt::FixInvalidRegPairOp(MachineBasicBlock &MBB,
                                           MachineBasicBlock::iterator &MBBI) {
   MachineInstr *MI = &*MBBI;
   unsigned Opcode = MI->getOpcode();
+  // FIXME: Code/comments below check Opcode == t2STRDi8, but this check returns
+  // if we see this opcode.
   if (Opcode != ARM::LDRD && Opcode != ARM::STRD && Opcode != ARM::t2LDRDi8)
     return false;
 
@@ -1615,8 +1615,8 @@ bool ARMLoadStoreOpt::FixInvalidRegPairOp(MachineBasicBlock &MBB,
   bool OddUndef = MI->getOperand(1).isUndef();
   bool BaseKill = BaseOp.isKill();
   bool BaseUndef = BaseOp.isUndef();
-  bool OffKill = isT2 ? false : MI->getOperand(3).isKill();
-  bool OffUndef = isT2 ? false : MI->getOperand(3).isUndef();
+  assert((isT2 || MI->getOperand(3).getReg() == ARM::NoRegister) &&
+         "register offset not handled below");
   int OffImm = getMemoryOpOffset(*MI);
   unsigned PredReg = 0;
   ARMCC::CondCodes Pred = getInstrPredicate(*MI, PredReg);
@@ -1654,21 +1654,14 @@ bool ARMLoadStoreOpt::FixInvalidRegPairOp(MachineBasicBlock &MBB,
     unsigned NewOpc2 = (isLd)
       ? (isT2 ? (OffImm+4 < 0 ? ARM::t2LDRi8 : ARM::t2LDRi12) : ARM::LDRi12)
       : (isT2 ? (OffImm+4 < 0 ? ARM::t2STRi8 : ARM::t2STRi12) : ARM::STRi12);
-    DebugLoc dl = MBBI->getDebugLoc();
-    // If this is a load and base register is killed, it may have been
-    // re-defed by the load, make sure the first load does not clobber it.
-    if (isLd &&
-        (BaseKill || OffKill) &&
-        (TRI->regsOverlap(EvenReg, BaseReg))) {
+    // If this is a load, make sure the first load does not clobber the base
+    // register before the second load reads it.
+    if (isLd && TRI->regsOverlap(EvenReg, BaseReg)) {
       assert(!TRI->regsOverlap(OddReg, BaseReg));
-      InsertLDR_STR(MBB, MBBI, OffImm+4, isLd, dl, NewOpc2,
-                    OddReg, OddDeadKill, false,
-                    BaseReg, false, BaseUndef, false, OffUndef,
-                    Pred, PredReg, TII, isT2);
-      InsertLDR_STR(MBB, MBBI, OffImm, isLd, dl, NewOpc,
-                    EvenReg, EvenDeadKill, false,
-                    BaseReg, BaseKill, BaseUndef, OffKill, OffUndef,
-                    Pred, PredReg, TII, isT2);
+      InsertLDR_STR(MBB, MBBI, OffImm + 4, isLd, NewOpc2, OddReg, OddDeadKill,
+                    false, BaseReg, false, BaseUndef, Pred, PredReg, TII);
+      InsertLDR_STR(MBB, MBBI, OffImm, isLd, NewOpc, EvenReg, EvenDeadKill,
+                    false, BaseReg, BaseKill, BaseUndef, Pred, PredReg, TII);
     } else {
       if (OddReg == EvenReg && EvenDeadKill) {
         // If the two source operands are the same, the kill marker is
@@ -1680,14 +1673,10 @@ bool ARMLoadStoreOpt::FixInvalidRegPairOp(MachineBasicBlock &MBB,
       // Never kill the base register in the first instruction.
       if (EvenReg == BaseReg)
         EvenDeadKill = false;
-      InsertLDR_STR(MBB, MBBI, OffImm, isLd, dl, NewOpc,
-                    EvenReg, EvenDeadKill, EvenUndef,
-                    BaseReg, false, BaseUndef, false, OffUndef,
-                    Pred, PredReg, TII, isT2);
-      InsertLDR_STR(MBB, MBBI, OffImm+4, isLd, dl, NewOpc2,
-                    OddReg, OddDeadKill, OddUndef,
-                    BaseReg, BaseKill, BaseUndef, OffKill, OffUndef,
-                    Pred, PredReg, TII, isT2);
+      InsertLDR_STR(MBB, MBBI, OffImm, isLd, NewOpc, EvenReg, EvenDeadKill,
+                    EvenUndef, BaseReg, false, BaseUndef, Pred, PredReg, TII);
+      InsertLDR_STR(MBB, MBBI, OffImm + 4, isLd, NewOpc2, OddReg, OddDeadKill,
+                    OddUndef, BaseReg, BaseKill, BaseUndef, Pred, PredReg, TII);
     }
     if (isLd)
       ++NumLDRD2LDR;
diff --git a/test/CodeGen/ARM/load_store_opt_kill.mir b/test/CodeGen/ARM/load_store_opt_kill.mir
new file mode 100644
index 00000000000..4c210eaf8e9
--- /dev/null
+++ b/test/CodeGen/ARM/load_store_opt_kill.mir
@@ -0,0 +1,12 @@
+# RUN: llc -mtriple=armv7-none-eabi -verify-machineinstrs -run-pass arm-ldst-opt %s -o - | FileCheck %s
+---
+# CHECK-LABEL: name: f
+name:            f
+# Make sure the load into %r0 doesn't clobber the base register before the second load uses it.
+# CHECK: %r3 = LDRi12 %r0, 12, 14, _
+# CHECK-NEXT: %r0 = LDRi12 %r0, 8, 14, _
+body:             |
+  bb.0:
+    liveins: %r0, %r3
+    %r0, %r3 = LDRD %r0, %noreg, 8, 14, %noreg
+...