[PATCH] D60565: [LOOPINFO] Extend Loop object to add utilities to get the loop bounds, step, induction variable, and guard branch.
Kit Barton via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 30 08:34:00 PDT 2019
kbarton requested changes to this revision.
kbarton added inline comments.
This revision now requires changes to proceed.
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:620
+
+ /// Return the loop induction variable if found, else return nullptr.
+ /// An instruction is considered as induction variable if
----------------
Whitney wrote:
> jdoerfert wrote:
> > Whitney wrote:
> > > jdoerfert wrote:
> > > > "The" loop induction variable, or "any" loop induction variable. There could easily be two or more.
> > > In this context, loop induction variable is defined as the the phi node in the loop header which is used in the condition of the conditional branch in the loop latch, so should only be one.
> > ```
> > for (i = 0, j = N; i < j; i++,j--)
> > body(i,j);
> > ```
> That's true. I edited the function description to specify that the loop induction variable is expected to be used as the LHS of the conditional branch in the loop latch.
Is it strictly necessary that the IV be the LHS?
What if you write:
```
(for i=0; N>i; i++)
```
Does LLVM switch it back to always be on the LHS?
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:591
+
+ /// Get the instruction which update the loop induction variable.
+ Instruction &getStepInst() const {
----------------
"which update" -> "that updates"
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:593
+ Instruction &getStepInst() const {
+ assert(StepInst && "Expecing valid step instrucion");
+ return *StepInst;
----------------
instrucion -> instruction
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:597
+
+ /// Get the step that the loop induction variable get updated for each loop
+ /// iteration.
----------------
"get updated for" -> "gets updated by in"
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:599
+ /// iteration.
+ Value &getStepValue() const { return *getStepInst().getOperand(1); }
+
----------------
This is assuming the step instruction has a very specific form. Is it safe to assume this all the time (it may be and I just haven't got to that part of the code yet).
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:602
+ /// Get the final value of the loop induction variable.
+ Value &getFinalIVValue() const { return *CmpInst.getOperand(1); }
+
----------------
Same - this is assuming a very specific format for the compare. Is it guaranteed to be true in all cases?
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:606
+ /// - the first successor of the latch branch is the loop header
+ /// - the LHS of the latch comparison is StepInst
+ ICmpInst::Predicate getCanonicalPredicate() const;
----------------
Can you clarify this comment?
Does this method assume:
1. The first successor of the latch branch is the loop header
2. The LHS of the latch comparison is the StepInst?
and if both of these conditions are not met, then what does it return?
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:620
+ // Set the initial value of the loop induction variable, given the loop
+ // induction variable.
+ void setInitialIVValue(PHINode &IndVar);
----------------
Can IndVar be made const?
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:623
+
+ // Set the instruction which update the loop induction variable, given the
+ // loop induction variable.
----------------
"which update" -> "that updates"
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:628
+ /// Get the direction of the loop using loop bounds. This only works for
+ /// constant bounds.
+ Direction getDirectionFromConstantBounds() const;
----------------
Can you clarify this comment?
If the bounds are not constant, it should return Unknown right?
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:634
+ // the initial value of the loop induction variable
+ Value *Init;
+
----------------
This is minor, but I think a more descriptive variable name would be useful here. Something like InitialIVValue, or even InitialValue.
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:652
+ /// - it is the closest conditional branch before the loop
+ /// - one successor dominates the loop preheader
+ /// - a loop exit block dominates the other successor
----------------
I would have expected the guard to dominate the preheader, and not have another block between the guard and the preheader. Or maybe I misunderstand this point.
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:653
+ /// - one successor dominates the loop preheader
+ /// - a loop exit block dominates the other successor
+ /// - the condition is to verify loop initial value satisfy the latch
----------------
"a" loop exit block, or "the" loop exit block?
Do these methods work on loops that have multiple exit blocks?
If they do work on loops with multiple exit blocks, then I would think that **all** loop exit blocks need to dominate the other successor.
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:655
+ /// - the condition is to verify loop initial value satisfy the latch
+ /// condition
+ BranchInst *getGuard(ScalarEvolution &SE) const;
----------------
What do you mean the loop initial value?
I think what you're trying to say is guard condition guarantees the loop body should be executed at least once. The only way I can think to say that using the current terminology is the lower bound of the loop is less than the upper bound (although that may not cover all possible conditions).
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:664
+ /// invariant.
+ PHINode *getInductionVariable(ScalarEvolution &SE,
+ bool CheckStepInvariant = false) const;
----------------
I find this very confusing.
I would have expected the getInductionVariable to return a Value, not an instruction. Strictly speaking, I don't think a PHINode is considered a variable.
What is the utility of returning the PHINode here instead of the actual Value?
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:665
+ PHINode *getInductionVariable(ScalarEvolution &SE,
+ bool CheckStepInvariant = false) const;
+
----------------
I'm not a big fan of boolean parameters. They're generally an indication that you're doing too much in the method and it should be split up into two separate methods.
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:668
+ /// Get the loop induction descriptor for the loop induction variable. Return
+ /// true if the loop induction varaible is found.
+ bool getInductionDescriptor(ScalarEvolution &SE,
----------------
varaible -> variable
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:676
+ /// - incremented by a loop invariant step for each loop iteration
+ /// - step instruction opcode should be add or sub
+ /// Note: auxiliary induction variable is not required to be used in the
----------------
This seems to be more restrictive then the check for induction variable.
Specifically, not being used outside the loop and the step instruction being an add or sub.
================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:678
+ /// Note: auxiliary induction variable is not required to be used in the
+ /// conditional branch in the loop latch.
+ bool isAuxiliaryInductionVariable(PHINode &AuxIndVar) const;
----------------
The auxiliary induction variable is not required to be used or cannot be used?
What happens if it is used in the conditional branch? Can that happen?
================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:226
+ if (D == Direction::Increasing)
+ return ICmpInst::ICMP_SLT;
+ if (D == Direction::Decreasing)
----------------
How do you know to use the signed version, instead of the unsigned version?
================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:226
+ if (D == Direction::Increasing)
+ return ICmpInst::ICMP_SLT;
+ if (D == Direction::Decreasing)
----------------
kbarton wrote:
> How do you know to use the signed version, instead of the unsigned version?
Do you handle the ULE/SLE case anywhere?
================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:289
+/// Return the closest conditional branch before loop L, where
+/// one successor dominates the loop preheader,
+/// and a loop exit block dominates the other successor.
----------------
I'm confused why one successor of the guard should dominate the preheader, not be the preheader itself.
================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:331
+ Succ = GuardBB;
+ GuardBB = GuardBB->getSinglePredecessor();
+ if (!GuardBB)
----------------
This will miss the case where a basic block has multiple predecessors, but all of the predecessors are to the same block.
I don't think this is a big deal, just pointing it out. I think if you've run simplifycfg prior to this, that should get cleaned up.
================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:370
+ SmallPtrSet<BasicBlock *, 4> Visited;
+ while (ExitBlock && ExitBlock != SkipToBlock &&
+ Visited.count(ExitBlock) == 0) {
----------------
Is it possible for getUniqueExitBlocks to add null pointers into the ExitBlocks?
I would have thought this is not possible (i.e., something is wrong if that's happening). Maybe it would be better to assert(ExitBlock && "Expecting valid ExitBlock") before the while loop to catch this if it happens.
================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:375
+ }
+ if (ExitBlock == SkipToBlock)
+ return GuardBI;
----------------
This is just checking that one of the exit blocks goes to the "other" successor of the guard.
Do you not need to check that **all** of the exit blocks go to that block?
================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:453
+
+ // the loop induction variable should not be loop invariant
+ if (isLoopInvariant(CmpInst->getOperand(0)))
----------------
How do you know the induction variable is always operand 0 of CmpInst?
================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:468
+ PHINode *IndVar = nullptr;
+ for (BasicBlock::iterator I = Header->begin(); isa<PHINode>(I); ++I) {
+ PHINode *PN = cast<PHINode>(I);
----------------
Can you use a phi iterator here instead?
for (PHINode &PN : Header->phis()) { ... }
================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:471
+ Value *IncomingValue = PN->getIncomingValueForBlock(Latch);
+ assert(IncomingValue && "Expecting valid incoming value from latch");
+ if (StepInst == IncomingValue) {
----------------
Doesn't this assert mean the only phis you expect to find in the header are from the latch block?
Is that always going to be true?
================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:523
+
+ // incremented by a loop invariant step fpr each loop iteration
+ // and the step instruction opcode should be add or sub.
----------------
fpr -> for ?
If not, what does fpr stand for?
================
Comment at: llvm/unittests/Analysis/LoopInfoTest.cpp:33
+/// Build the loop info and scalar evolution for the function and run the Test.
+static void runWithLoopInfoAndSE(
+ Module &M, StringRef FuncName,
----------------
Did clang-format format this for you?
The formatting is different then the formatting above.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60565/new/
https://reviews.llvm.org/D60565
More information about the llvm-commits
mailing list