[PATCH] D83392: Strlen loop idiom recognition

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1609
+  // It should have a precondition block where the generated strlen
+  // function call can be inserted.
+  auto *PreCondBB = PH->getSinglePredecessor();
----------------
I don't understand this bit about the precondition block; why are we not inserting the strlen call in the preheader?


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1627
+  if (!LoopLoad)
+    return false;
+
----------------
You probably want to check that the load is in address-space zero.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1641
+  for (BasicBlock::iterator I = LoopBody->begin(), E = LoopBody->end();
+       I != E;) {
+    Instruction *Inst = &*I++;
----------------
`for (auto *I : *LoopBody) {`



================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1648
+  // Check that the loop exit block is valid:
+  // It needs to have one LCSSA Phi
+  auto *LoopExitBB =
----------------
This checks there's at least one PHI, but not exactly one PHI.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1657
+  // Find the 'sub' instruction that computes the strlen. First we need
+  // the base pointer to check the operands of the 'sub'.
+  auto *BasePtrSCEV = dyn_cast<SCEVUnknown>(SE->getPointerBase(LoadEv));
----------------
I don't understand what you're doing here.

Probably the simplest thing to do here would be to directly expand LCSSAPHI based on the result of strlen, instead of trying to dig into the exit block and find a subtraction operation.


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

https://reviews.llvm.org/D83392