[ReachingDefs] Make isSafeToMove more strict.

[samparker]

Summary

Test that we're not moving the instruction through instructions with side-effects.

[SjoerdMeijer]

llvm/lib/CodeGen/ReachingDefAnalysis.cpp

404

I was just looking at the similar (but not the same) check in isSafeToRemove.
Just quickly checking: that one is also checking for isReturn, do we need that here? And here we check for isCall, do we need that there?

[samparker]

Yeah, I'll move these out into a helper function.

[samparker]

Created a helper which queries all the suspicious flags.

[SjoerdMeijer]

Cheers, nice one

llvm/lib/CodeGen/ReachingDefAnalysis.cpp

MachineInstr* ReachingDefAnalysis::getLocalLiveOutMIDef(MachineBasicBlock *MBB,
   int Def = getReachingDef(Last, PhysReg);
   for (auto &MO : Last->operands())
     if (MO.isReg() && MO.isDef() && MO.getReg() == PhysReg)
       return Last;
 
   return Def < 0 ? nullptr : getInstFromId(MBB, Def);
 }
 
+static bool mayHaveSideEffects(MachineInstr &MI) {
+  return MI.mayLoadOrStore() || MI.mayRaiseFPException() ||
+         MI.hasUnmodeledSideEffects() || MI.isTerminator() ||
+         MI.isCall() || MI.isBarrier() || MI.isBranch() || MI.isReturn();
+}
+
 // Can we safely move 'From' to just before 'To'? To satisfy this, 'From' must
 // not define a register that is used by any instructions, after and including,
 // 'To'. These instructions also must not redefine any of Froms operands.
 template<typename Iterator>
 bool ReachingDefAnalysis::isSafeToMove(MachineInstr *From,
                                        MachineInstr *To) const {
   if (From->getParent() != To->getParent())
     return false;
 
   SmallSet<int, 2> Defs;
   // First check that From would compute the same value if moved.
   for (auto &MO : From->operands()) {
     if (!MO.isReg() || MO.isUndef() || !MO.getReg())
       continue;
     if (MO.isDef())
       Defs.insert(MO.getReg());
     else if (!hasSameReachingDef(From, To, MO.getReg()))
       return false;
   }
 
   // Now walk checking that the rest of the instructions will compute the same
-  // value.
+  // value and that we're not overwriting anything. Don't move the instruction
+  // past any memory, control-flow or other ambigious instructions.
   for (auto I = ++Iterator(From), E = Iterator(To); I != E; ++I) {
+    if (mayHaveSideEffects(*I))
+      return false;
     for (auto &MO : I->operands())
-      if (MO.isReg() && MO.getReg() && MO.isUse() && Defs.count(MO.getReg()))
+      if (MO.isReg() && MO.getReg() && Defs.count(MO.getReg()))
         return false;
   }
   return true;
 }
 
 bool ReachingDefAnalysis::isSafeToMoveForwards(MachineInstr *From,
                                                MachineInstr *To) const {
   return isSafeToMove<MachineBasicBlock::reverse_iterator>(From, To);
ReachingDefAnalysis::isSafeToRemove(MachineInstr *MI, InstSet &ToRemove,
   return isSafeToRemove(MI, Visited, ToRemove, Ignore);
 }
 
 bool
 ReachingDefAnalysis::isSafeToRemove(MachineInstr *MI, InstSet &Visited,
                                     InstSet &ToRemove, InstSet &Ignore) const {
   if (Visited.count(MI) || Ignore.count(MI))
     return true;
-  else if (MI->mayLoadOrStore() || MI->hasUnmodeledSideEffects() ||
-           MI->isBranch() || MI->isTerminator() || MI->isReturn()) {
+  else if (mayHaveSideEffects(*MI)) {
     // Unless told to ignore the instruction, don't remove anything which has
     // side effects.
     return false;
   }
 
   Visited.insert(MI);
   for (auto &MO : MI->operands()) {
     if (!MO.isReg() || MO.isUse() || MO.getReg() == 0)

llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp

bool LowOverheadLoop::ValidateTailPredicate(MachineInstr *StartInsertPt) {
   if (RDA->hasLocalDefBefore(VCTP, NumElements)) {
     LLVM_DEBUG(dbgs() << "ARM Loops: VCTP operand is defined in the loop.\n");
     return false;
   }
 
   // The element count register maybe defined after InsertPt, in which case we
   // need to try to move either InsertPt or the def so that the [w|d]lstp can
   // use the value.
+  // TODO: On failing to move an instruction, check if the count is provided by
+  // a mov and whether we can use the mov operand directly.
   MachineBasicBlock *InsertBB = StartInsertPt->getParent();
   if (!RDA->isReachingDefLiveOut(StartInsertPt, NumElements)) {
     if (auto *ElemDef = RDA->getLocalLiveOutMIDef(InsertBB, NumElements)) {
       if (RDA->isSafeToMoveForwards(ElemDef, StartInsertPt)) {
         ElemDef->removeFromParent();
         InsertBB->insert(MachineBasicBlock::iterator(StartInsertPt), ElemDef);
         LLVM_DEBUG(dbgs() << "ARM Loops: Moved element count def: "
                    << *ElemDef);

llvm/test/CodeGen/Thumb2/LowOverheadLoops/move-def-before-start.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 # RUN: llc -mtriple=thumbv8.1m.main -mattr=+mve -run-pass=arm-low-overhead-loops --verify-machineinstrs %s -o - | FileCheck %s
 
 # Test that, though the vctp operand is defined at the end of the block,
 # that the correct value is used for the dlstp.
-# TODO: The pass currently just bails instead of finding the correct
-# value.
 
 --- |
   define dso_local arm_aapcs_vfpcc void @start_before_elems(i32* noalias nocapture %a, i8* nocapture readonly %b, i8* nocapture readonly %c, i32 %N) local_unnamed_addr #0 {
   entry:
     %div = lshr i32 %N, 1
     %cmp9 = icmp eq i32 %div, 0
     %0 = add nuw i32 %div, 3
     %1 = lshr i32 %0, 2