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

[Sanjoy Das via llvm-commits]

sanjoy added a subscriber: sanjoy.

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:64
@@ -61,2 +63,3 @@
 using namespace llvm;
+using namespace llvm::PatternMatch;
 
----------------
I'd open the namespace in just the one function that uses it.

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:205
@@ -198,2 +204,3 @@
       *CurLoop->getHeader()->getParent());
+  this->LPM = &LPM;
 
----------------
Why `this->`?

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

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1045
@@ +1044,3 @@
+    return false;
+  if (Ptr->getType() != Type::getInt8PtrTy(Ptr->getContext()))
+    return false;
----------------
Why not `isIntegerTy(8)`?

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1047
@@ +1046,3 @@
+    return false;
+  GetElementPtrInst *PtrGEP = dyn_cast<GetElementPtrInst>(Ptr);
+  auto &DL = CurLoop->getHeader()->getModule()->getDataLayout();
----------------
`auto *` might be cleaner.

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1052
@@ +1051,3 @@
+    return false;
+  PHINode *PtrPHI = dyn_cast<PHINode>(PtrGEP->getPointerOperand());
+  if (!PtrPHI || PtrPHI->getIncomingValueForBlock(LoopBody) != PtrGEP)
----------------
`auto *`?

================
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()) {
----------------
Why not use a range for here?

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1072
@@ +1071,3 @@
+      if (!CurLoop->isLoopInvariant(Start))
+        return false;
+      Value *Step;
----------------
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.

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1083
@@ +1082,3 @@
+    case Instruction::Add:
+    case Instruction::GetElementPtr:
+    case Instruction::ZExt:
----------------
What if these have uses outside the loop?  Also why these specifically?  Why not also handle, say, `mul`?

================
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));
----------------
Why not use a range for loop?

================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1089
@@ +1088,3 @@
+        Instruction *Op = dyn_cast<Instruction>(Inst->getOperand(i));
+        if (!Op) continue;
+        if (Op->getParent() != LoopBody)
----------------
Why not

```
if (auto *OpI = dyn_cast<..)
  if (OpI->getParent() != ...
    ...
```

Alternatively, why not use `Loop::hasLoopInvariantOperands` ?


Repository:
  rL LLVM

http://reviews.llvm.org/D12139