[PATCH] D70009: [ARM][ReachingDefAnalysis] Use RDA for loloops

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/CodeGen/ReachingDefAnalysis.cpp:207
+
+  for (auto &MI : *MBB) {
+    if (InstIds.count(&MI) && InstIds[&MI] == InstId)
----------------
You can drop the brackets


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:38
 
   class ARMLowOverheadLoops : public MachineFunctionPass {
     MachineFunction           *MF = nullptr;
----------------
Nit: This needs a clang format.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:125
 // Is it safe to define LR with DLS/WLS?
 // LR can defined if it is the operand to start, because it's the same value,
 // or if it's going to be equivalent to the operand to Start.
----------------
"LR can be defined"


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:142
   MachineBasicBlock *MBB = Start->getParent();
-  unsigned CountReg = Start->getOperand(0).getReg();
-  // Walk forward and backward in the block to find the closest instructions
-  // that define LR. Then also filter them out if they're not a mov lr.
-  MachineInstr *PredLRDef = SearchForDef(Start, MBB->rend(), ARM::LR);
-  if (PredLRDef && !IsMoveLR(PredLRDef, CountReg))
-    PredLRDef = nullptr;
-
-  MachineInstr *SuccLRDef = SearchForDef(Start, MBB->end(), ARM::LR);
-  if (SuccLRDef && !IsMoveLR(SuccLRDef, CountReg))
-    SuccLRDef = nullptr;
-
-  // We've either found one, two or none mov lr instructions... Now figure out
-  // if they are performing the equilvant mov that the Start instruction will.
-  // Do this by scanning forward and backward to see if there's a def of the
-  // register holding the count value. If we find a suitable def, return it as
-  // the insert point. Later, if InsertPt != Start, then we can remove the
-  // redundant instruction.
-  if (SuccLRDef) {
-    MachineBasicBlock::iterator End(SuccLRDef);
-    if (!SearchForDef(Start, End, CountReg)) {
-      return SuccLRDef;
-    } else
-      SuccLRDef = nullptr;
+  const int StartId = RDA->getInstId(Start);
+
----------------
Something like an int is usually not made const, unless it's a reference. But that's just a style preference.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:149
+      const int CountDefId = RDA->getReachingDef(Start, CountReg);
+      const int LRDefId = RDA->getReachingDef(Start, ARM::LR);
+      if (CountDefId < LRDefId || CountDefId > StartId)
----------------
This is getInstId(LRDef)?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:159
+      const int CountDefId = RDA->getReachingDef(LRDef, CountReg);
+      const int LRDefId = RDA->getReachingDef(Start, ARM::LR);
+      if (CountDefId > LRDefId || CountDefId < StartId)
----------------
Is this one correct? (start as opposed to back)


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:358
+  if (SetFlags) {
+    if (RDA->getReachingDef(&MBB->back(), ARM::CPSR) > RDA->getInstId(MI))
+      SetFlags = false;
----------------
Does this change need the Defs = [CPSR] for t2LoopDec change? Are there some tests for these cases that didn't change?


================
Comment at: llvm/test/CodeGen/Thumb2/ifcvt-neon-deprecated.mir:46
-    $d0 = VLDRD killed $r0, 0, 14, $noreg
-    ; Verify that the neon instruction VMOVv2i32 hasn't been conditionalized,
-    ; but the VLDR instruction that is available both in the VFP and Advanced
----------------
Is this part of the test still important?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70009/new/

https://reviews.llvm.org/D70009