[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:159
+bool AArch64LoopIdiomRecognize::run(Loop *L) {
+  CurLoop = L;
+
----------------
Do we need to check `skipLoop` for opt-bisect-limit?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoopIdiomRecognize.cpp:206
+  auto *EntryBI = dyn_cast<BranchInst>(PH->getTerminator());
+  if (!EntryBI || EntryBI->isConditional())
+    return false;
----------------
Doesn't being a "preheader" guarantee it's not conditional?


================
Comment at: llvm/lib/Target/AArch64/AArch64LoopIdiomRecognize.cpp:306
+
+  // Check we are loading i8 values from two loop invariant pointers
+  if (!CurLoop->isLoopInvariant(PtrA) || !CurLoop->isLoopInvariant(PtrB) ||
----------------
This doesn't guarantee the loads are loading i8. The load have their own type and don't have to match the GEP result type.


================
Comment at: llvm/lib/Target/AArch64/AArch64LoopIdiomRecognize.cpp:321
+
+  if (IdxA != IdxB || !match(IdxA, m_ZExt(m_Instruction(Index))))
+    return false;
----------------
m_Instruction -> m_Specific


================
Comment at: llvm/lib/Target/AArch64/AArch64LoopIdiomRecognize.cpp:326
+  // add 1 to the start value.
+  bool IncIdx = DT->dominates(Index, cast<Instruction>(IdxA));
+
----------------
Isn't IdxA, zext(Index)? So Index must dominate IdxA.


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

https://reviews.llvm.org/D158291