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

[Aleksandr Popov via Phabricator via llvm-commits]

aleksandr.popov added inline comments.


================
Comment at: llvm/lib/Analysis/GuardUtils.cpp:123
+  SmallPtrSet<Value *, 4> Visited;
+  Visited.insert(Condition);
+  do {
----------------
anna wrote:
> 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());
> ```
Do you mind if I do that  refactoring in the next separate patch? 
I've intended just to extract logic from LoopPredication::collectChecks and move it here straightforwardly.


================
Comment at: llvm/lib/Analysis/GuardUtils.cpp:145
+
+  parseCondition(Condition, [&](Value *Check) -> bool {
+    if (!isWidenableCondition(Check))
----------------
anna wrote:
> 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. 
Yep, returning true was change in advance. I'll turn callback's type into void.
The reason why I need templated function for `parseCondition` - is an intent to separate condition parsing and checks collecting.
So in the `parseCondition` I just want to parse the condition while in the `parseWidenableGuard` I want to make a decision whether to add check in the resulting list or not (in case if it's widenable-condition)


================
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) {
----------------
anna wrote:
> Pls land this and the added debug statement in the test separately (can land without review), since it is unrelated to patch. 
Ok, will do it in the separate patch, thanks!


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

https://reviews.llvm.org/D157276