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

[gberry]

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.

[efriedma]

If I understand correctly, the original code was checking for the kill flags just to short-circuit the if statement more quickly?

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

1659

Extra parens.

[gberry]

In D37164#853040, @efriedma wrote:

If I understand correctly, the original code was checking for the kill flags just to short-circuit the if statement more quickly?

That's what it looks like to me: an attempt to reduce the calls to regsOverlap(). Or at least that would make sense for 'BaseKill'. Checking 'OffKill' I don't get unless 'Base' and 'Off' are somehow known to be related?

[MatzeB]

In D37164#854053, @gberry wrote:

In D37164#853040, @efriedma wrote:

If I understand correctly, the original code was checking for the kill flags just to short-circuit the if statement more quickly?

That's what it looks like to me: an attempt to reduce the calls to regsOverlap(). Or at least that would make sense for 'BaseKill'. Checking 'OffKill' I don't get unless 'Base' and 'Off' are somehow known to be related?

Indeed checking for the kill flags is odd and removing the checks looks good to me. However there may be something about the offset register: Won't we have the same problem when the first load overwrites the offset register? So maybe we need to check for an overlap with offsetreg as well (and have a bigger problem if base+offset get overridden?)

[MatzeB]

In D37164#854194, @MatzeB wrote:

In D37164#854053, @gberry wrote:

In D37164#853040, @efriedma wrote:

If I understand correctly, the original code was checking for the kill flags just to short-circuit the if statement more quickly?

That's what it looks like to me: an attempt to reduce the calls to regsOverlap(). Or at least that would make sense for 'BaseKill'. Checking 'OffKill' I don't get unless 'Base' and 'Off' are somehow known to be related?

Indeed checking for the kill flags is odd and removing the checks looks good to me. However there may be something about the offset register: Won't we have the same problem when the first load overwrites the offset register? So maybe we need to check for an overlap with offsetreg as well (and have a bigger problem if base+offset get overridden?)

This whole OffKill/OffUndef handling is confusing. We only ever query the kill and undef flag and don't even bother to query the register itself. We then pass the kill/undef values to InsertLDR_STR() which doesn't even use the parameter.

Maybe this is because t2LDRDi8 has an immediate offset and hence no offset register is used, while LDRD/STRD always have consecutive even/odd registers so we should always hit the OddRegNum > EvenRegNum && OffImm == 0 case earlier in the function in the cases where an offset register is present.

At least smells to me like OffUndef/OffKill uses could be replaced with assert(!isT2 || MI->getOperand(3).getReg() == 0);

[gberry]

Address review comments

[efriedma]

I don't think we ever form an ldrd/strd with a register offset. See ARMPreAllocLoadStoreOpt::CanFormLdStDWord.

[MatzeB]

LGTM

[gberry]

In D37164#854223, @MatzeB wrote:

In D37164#854194, @MatzeB wrote:

In D37164#854053, @gberry wrote:

In D37164#853040, @efriedma wrote:

If I understand correctly, the original code was checking for the kill flags just to short-circuit the if statement more quickly?

That's what it looks like to me: an attempt to reduce the calls to regsOverlap(). Or at least that would make sense for 'BaseKill'. Checking 'OffKill' I don't get unless 'Base' and 'Off' are somehow known to be related?

Indeed checking for the kill flags is odd and removing the checks looks good to me. However there may be something about the offset register: Won't we have the same problem when the first load overwrites the offset register? So maybe we need to check for an overlap with offsetreg as well (and have a bigger problem if base+offset get overridden?)

This whole OffKill/OffUndef handling is confusing. We only ever query the kill and undef flag and don't even bother to query the register itself. We then pass the kill/undef values to InsertLDR_STR() which doesn't even use the parameter.

Maybe this is because t2LDRDi8 has an immediate offset and hence no offset register is used, while LDRD/STRD always have consecutive even/odd registers so we should always hit the OddRegNum > EvenRegNum && OffImm == 0 case earlier in the function in the cases where an offset register is present.

At least smells to me like OffUndef/OffKill uses could be replaced with assert(!isT2 || MI->getOperand(3).getReg() == 0);

Yeah, I came to the same conclusion. I was unable to trigger this assert in any lit or test-suite test, so perhaps we are never using a register offset for this opcode?

lib/Target/ARM/ARMLoadStoreOptimizer.cpp

static bool isMemoryOp(const MachineInstr &MI) {
   if (MI.getOperand(1).isUndef())
     return false;
 
   return true;
 }
 
 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))
       .addReg(Reg, getDefRegState(true) | getDeadRegState(RegDeadKill))
       .addReg(BaseReg, getKillRegState(BaseKill)|getUndefRegState(BaseUndef));
     MIB.addImm(Offset).addImm(Pred).addReg(PredReg);
   } else {
     MachineInstrBuilder MIB = BuildMI(MBB, MBBI, MBBI->getDebugLoc(),
                                       TII->get(NewOpc))
       .addReg(Reg, getKillRegState(RegDeadKill) | getUndefRegState(RegUndef))
       .addReg(BaseReg, getKillRegState(BaseKill)|getUndefRegState(BaseUndef));
     MIB.addImm(Offset).addImm(Pred).addReg(PredReg);
   }
 }
 
 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;
 
   const MachineOperand &BaseOp = MI->getOperand(2);
   unsigned BaseReg = BaseOp.getReg();
   unsigned EvenReg = MI->getOperand(0).getReg();
   unsigned OddReg  = MI->getOperand(1).getReg();
   unsigned EvenRegNum = TRI->getDwarfRegNum(EvenReg, false);
bool ARMLoadStoreOpt::FixInvalidRegPairOp(MachineBasicBlock &MBB,
   bool EvenDeadKill = isLd ?
     MI->getOperand(0).isDead() : MI->getOperand(0).isKill();
   bool EvenUndef = MI->getOperand(0).isUndef();
   bool OddDeadKill  = isLd ?
     MI->getOperand(1).isDead() : MI->getOperand(1).isKill();
   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);
 
   if (OddRegNum > EvenRegNum && OffImm == 0) {
     // Ascending register numbers and no offset. It's safe to change it to a
     // ldm or stm.
     unsigned NewOpc = (isLd)
if (OddRegNum > EvenRegNum && OffImm == 0) {
     unsigned NewOpc = (isLd)
       ? (isT2 ? (OffImm < 0 ? ARM::t2LDRi8 : ARM::t2LDRi12) : ARM::LDRi12)
       : (isT2 ? (OffImm < 0 ? ARM::t2STRi8 : ARM::t2STRi12) : ARM::STRi12);
     // Be extra careful for thumb2. t2LDRi8 can't reference a zero offset,
     // so adjust and use t2LDRi12 here for that.
     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
         // probably on the first one. e.g.
         // t2STRDi8 %R5<kill>, %R5, %R9<kill>, 0, 14, %reg0
         EvenDeadKill = false;
         OddDeadKill = true;
       }
       // 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;
     else
       ++NumSTRD2STR;
   }
 
   MBBI = MBB.erase(MBBI);

test/CodeGen/ARM/load_store_opt_kill.mir

+# 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
+...