[PATCH] D157276: [NFC][LoopPredication] Extract guard parsing to GuardUtils

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: llvm/lib/Analysis/GuardUtils.cpp:121
+static void parseCondition(Value *Condition, CallbackType Callback) {
+  SmallVector<Value *, 4> Worklist(1, Condition);
+  SmallPtrSet<Value *, 4> Visited;
----------------
Pls leave default value for `SmallVector` (i.e. SmallVector<Value *> Worklist = {Condition};). If we happen to have more than 4 conditions, we end up resizing. 


================
Comment at: llvm/lib/Analysis/GuardUtils.cpp:123
+  SmallPtrSet<Value *, 4> Visited;
+  Visited.insert(Condition);
+  do {
----------------
You can delay adding `Condition` into `Visited` and make this entire loop as:
```
do {
    Value *Check = Worklist.pop_back_val();
     if (!Visited.insert(Check).second)
       continue;
    Value *LHS, *RHS;
    if (match(Check, m_And(m_Value(LHS), m_Value(RHS)))) {
        Worklist.push_back(LHS);
        Worklist.push_back(RHS);
      continue;
    }
    if (!Callback(Check))
      break;
  } while (!Worklist.empty());
```


================
Comment at: llvm/lib/Analysis/GuardUtils.cpp:145
+
+  parseCondition(Condition, [&](Value *Check) -> bool {
+    if (!isWidenableCondition(Check))
----------------
This always returns true. Why do you need a templated function for `parseCondition`  in this patch?  I'd suggest  the non-templated version and once you add support for parsing it in GuardWidening, we can see if we need a templated version or not. 


================
Comment at: llvm/lib/Transforms/Scalar/LoopPredication.cpp:776
+  parseWidenableGuard(Guard, Checks);
+  LLVM_DEBUG(dbgs() << "Found checks:\n");
+  std::for_each(Checks.begin(), Checks.end(), [](const Value *Check) {
----------------
Pls land this and the added debug statement in the test separately (can land without review), since it is unrelated to patch. 


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

https://reviews.llvm.org/D157276