[PATCH] D71837: [ARM][MVE] Tail Predicate IsSafeToRemove

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

A round of nits:



================
Comment at: llvm/include/llvm/CodeGen/ReachingDefAnalysis.h:137
+
+  /// Collect the the users of the value stored in PhysReg, which is defined
+  /// by MI.
----------------
typo: the the


================
Comment at: llvm/lib/CodeGen/ReachingDefAnalysis.cpp:262
+
+void ReachingDefAnalysis::getAllUses(MachineInstr *MI, int PhysReg,
+                                     SmallPtrSetImpl<MachineInstr*> &Uses) {
----------------
How about `getReachingGlobalUses()` because it is more consistent with `getReachingLocalUses()` that we already have and also more explicit about its "scope" of all uses.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:385
+           MI->isBranch() || MI->isTerminator() || MI->isReturn())
+    // Unless told to ignore the instruction, don't remove anything which has
+    // side effects.
----------------
nit: perhaps a debug message here that MI has side-effects and can't be removed


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:863
 
 MachineInstr* ARMLowOverheadLoops::ExpandLoopStart(LowOverheadLoop &LoLoop) {
+  LLVM_DEBUG(dbgs() << "ARM Loops: Expanding LoopStart.\n");
----------------
nit: `MachineInstr* A...` -> `MachineInstr *A...`


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:868
+  if (LoLoop.IsTailPredicationLegal()) {
+    SmallVector<MachineInstr*, 4> Killed;
+    SmallVector<MachineInstr*, 4> Dead;
----------------
I was wondering if it was a bit counter intuitive to start this `ExpandLoopStart` function with removing loop iteration instructions. We have `RemoveDeadBranch` that is called after `ExpandLoopStart`, so perhaps a similar helper function for this?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:871
+    if (auto *Def = RDA->getReachingMIDef(LoLoop.Start,
+                                      LoLoop.Start->getOperand(0).getReg())) {
+      SmallPtrSet<MachineInstr*, 4> Visited;
----------------
nit: indentation


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

https://reviews.llvm.org/D71837