[PATCH] D38494: [SCEV] Handling for ICmp occuring in the evolution chain.
Sanjoy Das via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 30 00:03:59 PDT 2017
sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.
This is looking close to ready, just a few minor things inline.
================
Comment at: lib/Analysis/ScalarEvolution.cpp:4092
+ BasicBlock *Latch = nullptr;
+ if (L && (Latch = L->getLoopLatch())) {
+ BranchInst *BI = dyn_cast<BranchInst>(Latch->getTerminator());
----------------
Doesn't look like `L` can be `nullptr` - `ScalarEvolution::createAddRecFromPHI` bails out early if `L` is `nullptr`.
I think you can rewrite this to be:
```
if (BasicBlock *Latch = ...) {
...
}
```
================
Comment at: lib/Analysis/ScalarEvolution.cpp:4115
+ case Instruction::Select: {
+ const SCEV *CondSE = visit(SE.getSCEV(I->getOperand(0)));
+ if (isa<SCEVConstant>(CondSE)) {
----------------
Calling `getSCEV` + `visit` seems a bit overkill -- can't you just compare `I->getOperand(0)` with `LoopBackOnPosCond` here?
How about adding an `Optional<bool> isValueKnown(Value *Cond)` function that returns `true` or `false` if `Cond` is equal to `LoopBackOnPosCond` else returns `None`, and using that function in both the cases?
================
Comment at: lib/Analysis/ScalarEvolution.cpp:4122
+ }
+ } break;
+ default: {
----------------
Typically LLVM style is would be putting the `break;` inside the curly brace block.
================
Comment at: lib/Analysis/ScalarEvolution.cpp:4137
+ // Set to true if loop back is on positive branch condition.
+ bool LoopBackOnPosCond;
+
----------------
A more idiomatic name for this is `BackedgeTakenOnTrue`.
================
Comment at: lib/Analysis/ScalarEvolution.cpp:6518
}
+
----------------
Whitespace damange?
================
Comment at: test/Analysis/ScalarEvolution/pr34538.ll:13
+ %cmp = icmp slt i32 %start.0, 10000
+ %inc = zext i1 %cmp to i32
+ %inc.start.0 = add nsw i32 %start.0, %inc
----------------
I'd put these two test cases in the same file with a somewhat more descriptive file name. Perhaps you can put the PR number as the function name instead?
https://reviews.llvm.org/D38494
More information about the llvm-commits
mailing list