[PATCH] D102234: [SimpleLoopBoundSplit] Split Bound of Loop which has conditional branch with IV
JinGu Kang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 18 07:38:07 PDT 2021
jaykang10 added inline comments.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:46
+ /// Signedness info
+ bool HasNoSignedWrap;
+
----------------
mkazantsev wrote:
> Why signed only? TODO unsigned?
You are right! I will add unsigned one too.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:62
+ Cond.AddRecValue = ICmp->getOperand(0);
+ Cond.BoundValue = ICmp->getOperand(1);
+ Cond.AddRecSCEV = SE.getSCEV(Cond.AddRecValue);
----------------
mkazantsev wrote:
> I think this is a natural place where you could apply PatternMatch.
Sorry... I am not sure how I can use PatternMatch to extract information from ICmp instruction...
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:72
+ Cond.OpsSwapped = true;
+ }
+ };
----------------
mkazantsev wrote:
> Assert that one of them is an addrec and another is available at loop entry?
The AddRec is checked on `HasProcessableCondition`. I will add code to check whether the bound is available at loop entry on `HasProcessableCondition`.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:75
+
+ auto CalculateUpperBoundWithLT = [&SE](ICmpInst::Predicate &Pred,
+ const SCEVAddRecExpr *&AddRecSCEV,
----------------
mkazantsev wrote:
> It looks natural to encapsulate Pred, AddRec, Bound and nowrap flags into a structure (e.g. some `BoundInfo`) and pass `const BoundInfo &` here. WDYT?
Yep, you are right. I could pass the `ConditionInfo` directly. I will update it.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:92
+ if (IsExitCond && Pred == ICmpInst::ICMP_EQ) {
+ Pred = AddRecSCEV->hasNoSignedWrap() ? ICmpInst::ICMP_SLT
+ : ICmpInst::ICMP_ULT;
----------------
mkazantsev wrote:
> 1. Why do you prefer signed predicates over unsigned? Other parts of LLVM (e.g. indvars) tend to canonicalize everything as unsigned if they can prove it.
> 2. Isn't `AddRecSCEV->hasNoSignedWrap()` a same thing as `HasNoSignedWrap`? If so, remove one of them.
> 1. Why do you prefer signed predicates over unsigned? Other parts of LLVM (e.g. indvars) tend to canonicalize everything as unsigned if they can prove it.
I just tried to follow the AddRec's flag. I will update the code with unsigned one.
> 2. Isn't `AddRecSCEV->hasNoSignedWrap()` a same thing as `HasNoSignedWrap`? If so, remove one of them.
Yep, I will remove the `AddRecSCEV->hasNoSignedWrap()`.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:102
+ // Range Range
+ // AddRec <= Bound --> AddRec < Bound - 1
+ if (Pred == ICmpInst::ICMP_ULE || Pred == ICmpInst::ICMP_SLE) {
----------------
mkazantsev wrote:
> 1. This is just wrong, it should be `AddRec < Bound + 1`.
> 2. It is only correct if you can prove that `Bound + 1` does not overflow. Simple exapmle is `X <= SINT_MAX` --> `X < SINT_MIN` is wrong.
> 1. This is just wrong, it should be `AddRec < Bound + 1`.
Oops, sorry. I will update it.
> 2. It is only correct if you can prove that `Bound + 1` does not overflow. Simple exapmle is `X <= SINT_MAX` --> `X < SINT_MIN` is wrong.
You are right!!! I will add code to check the overflow.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:120
+
+ const SCEV *StepRecSCEV = AddRecSCEV->getStepRecurrence(SE);
+ // Allowed constant step.
----------------
mkazantsev wrote:
> Check `isAffine` before it to save compile time in case of really complex `AddRecs`.
Yep, I will check it.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:127
+ // Allowed positive step.
+ if (StepCI->isNegative())
+ return false;
----------------
mkazantsev wrote:
> I understand that zero step is impossible, but still, being negative and being non-positive is't the same thing is general. :)
Yep, I will check zero step too.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:131
+ // Allowed step is one.
+ if (!StepCI->isOne())
+ return false;
----------------
mkazantsev wrote:
> 1. This check makes the check above meaningless.
> 2. Reading the code, I think the only place where it matters is replacing `Eq` condition with `Lt`. Do we beed this for something else?
> 3. Please add a TODO to add negative step support. We've stuggled a lot with it in IndVars, and I see this pain coming again.
>
> 1. This check makes the check above meaningless.
I agree.
> 2. Reading the code, I think the only place where it matters is replacing `Eq` condition with `Lt`. Do we beed this for something else?
No, it is for the `EQ` condition. I will update code to check the one step with `EQ` condition.
> 3. Please add a TODO to add negative step support. We've stuggled a lot with it in IndVars, and I see this pain coming again.
Yep, I will add the TODO for negative step.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:139
+ // by CalculateUpperBoundWithLT.
+ if (Cond.Pred != ICmpInst::ICMP_ULT && Cond.Pred != ICmpInst::ICMP_SLT)
+ return false;
----------------
mkazantsev wrote:
> Check this before you construct SCEVs to save compile time.
um... I think we need to check it after `CalculateUpperBoundWithLT` in order to check only `LT` condition... We could add more conversions of conditions like 'EQ` --> `LT', `LE' --> `LT` later. If I missed something, please let me know.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:160
+ // Check loop is in LCSSA form.
+ if (!L.isRecursivelyLCSSAForm(DT, LI))
+ return false;
----------------
mkazantsev wrote:
> It's innermost loop, so I thing "recursive" check is an overkill. Or do we plan to support outer loops for it?
You are right!!! I will update it with `isLCSSAForm`.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:168
+ BasicBlock *ExitingBB = L.getExitingBlock();
+ // Assumed only one exiting block.
+ if (!ExitingBB)
----------------
mkazantsev wrote:
> This is a very restrictive thing. Why is it required?
I wanted to make this pass as simple as possible at this stage to figure out basic problems. If we support multiple exiting blocks, we could have to consider below things.
1. Multiple exit conditions
- We could need more min/max operations to create new bounds.
2. Update first loop's exit blocks to preheader of second loop.
There could be something more to be considered.
If possible, I would like to consider multiple exits after finishing to support single exit.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:174
+ // Allowed only conditional branch.
+ if (!ExitingBI || !ExitingBI->isConditional() ||
+ isa<Constant>(ExitingBI->getCondition()) ||
----------------
mkazantsev wrote:
> Use pattern match.
Yep, I will update it with pattern match.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:185
+ // Check the condition is processable.
+ if (!HasProcessableCondition(ICmp, Cond, true))
+ return false;
----------------
mkazantsev wrote:
> Pls add `/*IsExitCond*/`
Yep, I will update it.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:220
+ if (FoundCandidate)
+ return false;
+
----------------
mkazantsev wrote:
> Logicaly, this check should go last. You may discard the candidate by conditions below.
> Anyways, why is that a problem at all? If we have at least one candidate we can get rid of it and then proceed with the other ones.
It is not problem. As I mentioned before, I wanted to make this pass as simple as possible at this stage to figure out basic problems. I will remove this check.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:229
+ if (!HasProcessableCondition(ICmp, SplitCandidateCond, false))
+ return false;
+
----------------
mkazantsev wrote:
> Why not `continue`?
Oops, You are right!!! I will update it.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:232
+ SplitCandidateCond.BI = BI;
+ FoundCandidate = true;
+ }
----------------
mkazantsev wrote:
> Break?
It was to avoid multiple split candidates. I will add break.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:239
+ // Assumed same signedness.
+ if (ExitingCond.HasNoSignedWrap != SplitCandidateCond.HasNoSignedWrap)
+ return false;
----------------
mkazantsev wrote:
> Do we ever check they are against the same AddRec?
> If it's supposed to be same, I think we can just assert on it.
> If not, I'm not catching how the rest of logic is going to work.
You are right. It is supposed to be same. However, we can not compare the AddRec directly because the AddRec of exit cond is different with split candidate's one because `inc++` is followed by exit cond. The start value of AddRec is different as below.
```
ExitCond AddRec
{1,+,1}<nuw><nsw><%loop>
SplitCandidateCond AddRec
{0,+,1}<nuw><nsw><%loop>
```
I think we have checked the step of AddRec and need to check its start value and signedness more to say they are same AddRec. I forgot to check the start value. I will add the check. If I missed something or you feel something wrong, please let me know.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:244
+ // This transformation aims mainly to help loop vectorizer. If we can not
+ // vectorize the loop, it is not profitable.
+ if (!LAI.canVectorizeMemory())
----------------
mkazantsev wrote:
> I think it's a too strong statement. Getting rid of a branch in a critical loop may be useful even without vectorization. If you want to be that restrictive, place add an option to ignore this check.
Yep, you are right. I will remove this check. I am checking performance impact with this pass. Maybe, I could add something more here.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:263
+ for (auto &Inst : *Succ) {
+ if (isa<LoadInst>(Inst) || isa<StoreInst>(Inst))
+ return true;
----------------
mkazantsev wrote:
> `any_of`?
> I believe we don't vectorize volatile load/stores (or do we?) Maybe check at least this.
> It looks like the transform is also profitable if one of these instructions is side exiting. We don't vectorize them so far, but if you get rid of it, the loop may become vectorizable.
> `any_of`?
> I believe we don't vectorize volatile load/stores (or do we?) Maybe check at least this.
Yep, I will update it.
> It looks like the transform is also profitable if one of these instructions is side exiting. We don't vectorize them so far, but if you get rid of it, the loop may become vectorizable.
um... if the condition of the side exit is related to induction variable, we could handle it.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:273
+
+ // Now, we have a split candidate. Let's split loop bound with it as below.
+ // +--------------------+
----------------
mkazantsev wrote:
> It looks like it can be an utility function usable by other parts of LLVM. Consider factoring it out.
Once the basic version of this pass is stabilized, I would consider it.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:337
+ const SCEV *NewBound1 =
+ SplitCandidateCond.HasNoSignedWrap
+ ? SE.getSMinExpr(ExitingCond.BoundSCEV, SplitCandidateCond.BoundSCEV)
----------------
mkazantsev wrote:
> Same question about preference of signed over unsigned. LLVM does otherwise in other parts. Let's not create more self-contradictions.
Yep, I will update it.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:411
+ simplifyLoop(&L, &DT, &LI, &SE, nullptr, nullptr, true);
+ formLCSSARecursively(*SecondLoop, DT, &LI, &SE);
+ simplifyLoop(SecondLoop, &DT, &LI, &SE, nullptr, nullptr, true);
----------------
mkazantsev wrote:
> This can be expensive. Can't we do a more surgical update?
>
> I'm fine if it's a TODO with follow-up fix.
>
Yep, I will add TODO and try to fix it later.
================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:435
+
+ assert(AR.DT.verify(DominatorTree::VerificationLevel::Fast));
+
----------------
mkazantsev wrote:
> Please verify loop info too.
Yep, I will update it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102234/new/
https://reviews.llvm.org/D102234
More information about the llvm-commits
mailing list