[PATCH] D157276: [NFC][LoopPredication] Extract guard parsing to GuardUtils
Aleksandr Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 9 03:30:33 PDT 2023
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
More information about the llvm-commits
mailing list