[PATCH] D12139: Add 'strlen' formation to LoopIdiomRecognize

[Nick Lewycky via llvm-commits]

nlewycky marked 7 inline comments as done.

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1043
@@ +1042,3 @@
+        (Ptr = cast<LoadInst>(Load)->getPointerOperand()) &&
+        cast<Instruction>(Ptr)->getParent() == LoopBody))
+    return false;
----------------
sanjoy wrote:
> Why not declare `Ptr` as a `LoadInst *`?  Then you won't need this second `cast<Instruction>(`.
Because then it can't be used in m_Value(Load).

Do you think I should I add an m_Load instead?

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1045
@@ +1044,3 @@
+    return false;
+  if (Ptr->getType() != Type::getInt8PtrTy(Ptr->getContext()))
+    return false;
----------------
sanjoy wrote:
> Why not `isIntegerTy(8)`?
'i8*' not 'i8'. I could do "cast<PointerType>(Ptr->getType())->getElementType()->isIntegerTy(8)" if you prefer?

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1057
@@ +1056,3 @@
+  // Check that this loop does nothing else each iteration.
+  for (Instruction *Inst = LoopBody->begin(); !isa<TerminatorInst>(Inst);
+       Inst = Inst->getNextNode()) {
----------------
sanjoy wrote:
> Why not use a range for here?
I wanted to exclude the terminator.

I could add a simple "if (isa<TerminatorInst>(Inst)) continue;".

I don't think we can use "E = llvm::prior(LoopBody->end())" because it may walk off the beginning of the block. At this point, we haven't yet shown that LoopCond, PtrGEP or PtrPHI are actually members instructions inside LoopBody.

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1072
@@ +1071,3 @@
+      if (!CurLoop->isLoopInvariant(Start))
+        return false;
+      Value *Step;
----------------
sanjoy wrote:
> How can `Start` ever not be loop invariant?
> 
> My reasoning went like this: it'll have to dominate the entry edge, and therefore the preheader, but if it is defined inside the only BB, then the only BB == the header dominates the preheader, making it part of the loop.
I concur. This is now an assertion.

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1081
@@ +1080,3 @@
+    }
+    switch (Inst->getOpcode()) {
+    case Instruction::Add:
----------------
hfinkel wrote:
> Do you also need to skip debug intrinsics, etc.?
Also, I'm not updating DebugLoc anywhere in this code. If there's debug intrinsics they'll move along with the splice, which is probably close enough to right, as much as any tracking for optimized debug info.

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1083
@@ +1082,3 @@
+    case Instruction::Add:
+    case Instruction::GetElementPtr:
+    case Instruction::ZExt:
----------------
sanjoy wrote:
> sanjoy wrote:
> > What if these have uses outside the loop?  Also why these specifically?  Why not also handle, say, `mul`?
> > What if these have uses outside the loop?
> 
> I'll redact this bit -- I had initially missed what you were doing with the splicing logic below.
Gave this some thought, decided we can safely extend it.

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1087
@@ +1086,3 @@
+    case Instruction::Trunc:
+      for (unsigned i = 0, e = Inst->getNumOperands(); i != e; ++i) {
+        Instruction *Op = dyn_cast<Instruction>(Inst->getOperand(i));
----------------
sanjoy wrote:
> Why not use a range for loop?
Not applicable.


Repository:
  rL LLVM

http://reviews.llvm.org/D12139