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

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1043
@@ +1042,3 @@
+        (Ptr = cast<LoadInst>(Load)->getPointerOperand()) &&
+        cast<Instruction>(Ptr)->getParent() == LoopBody))
+    return false;
----------------
nlewycky wrote:
> 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?
> Do you think I should I add an m_Load instead?

I think that's a good idea, it will be generally useful.

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

================
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()) {
----------------
nlewycky wrote:
> 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.
> I could add a simple "if (isa<TerminatorInst>(Inst)) continue;".

I mildly prefer that.

================
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));
----------------
nlewycky wrote:
> sanjoy wrote:
> > Why not use a range for loop?
> Not applicable.
I may be missing something here, but can't you do `for (Value *Op : Inst->operands())`?


Repository:
  rL LLVM

http://reviews.llvm.org/D12139