[PATCH] D158291: [PoC][WIP] Add an AArch64 specific pass for loop idiom recognition

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoopIdiomRecognize.cpp:289
+  // the pattern and find the GEP instructions used by the loads.
+  ICmpInst::Predicate WhilePred;
+  BasicBlock *FoundBB;
----------------
Do we know for sure that WhileBB is the block in the loop? Could EndBB above be the backedge?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoopIdiomRecognize.cpp:295
+             m_Br(m_ICmp(WhilePred, m_Value(LoadA), m_Value(LoadB)),
+                  m_BasicBlock(TrueBB), m_BasicBlock(FoundBB))))
+    return false;
----------------
Do we need to check that TrueBB is the header?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoopIdiomRecognize.cpp:328
+
+  if (IdxA != IdxB || !match(IdxA, m_ZExt(m_Specific(Index))))
+    return false;
----------------
The IdxA != IdxB check is identical to the previous if


================
Comment at: llvm/lib/Target/AArch64/AArch64LoopIdiomRecognize.cpp:333
+  // add 1 to the start value.
+  bool IncIdx = DT->dominates(Index, cast<Instruction>(IdxA));
+
----------------
`IdxA` is is a zero extend of `Index` according to the previous if, so doesn't Index always dominate IdxA?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoopIdiomRecognize.cpp:274
+  // Don't replace the loop if the add has a wrap flag.
+  if (Index->hasNoSignedWrap() || Index->hasNoUnsignedWrap())
+    return false;
----------------
why is this needed?


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

https://reviews.llvm.org/D158291