[PATCH] D111353: [SCEV] Extend ability to infer flags to more complicates scopes
Max Kazantsev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 11 21:13:56 PDT 2021
mkazantsev added a comment.
I'm generally OK now, but please give your opinion on forgetLoop question since there might be a bug lurking here.
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6636
+isGuaranteedToTransferExecutionToSuccessor(const BasicBlock *BB) {
+ assert(LI.getLoopFor(BB) && "must be in a loop for invalidation!");
+ if (!BlockTransferExecutionToSuccessorCache.count(BB))
----------------
This is a really strange and counter-intuitive limitation. I'm OK with it for now, but I think we might want to remove it in the future. Should we add a TODO here or in method description?
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6660
+ // require reachability
+ if (!DT.isReachableFromEntry(B->getParent()))
+ return false;
----------------
Makes sense to check reachibility of `A` as well, just to avoid useless work and save some CT.
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6675
+ auto *PrevBB = BB->getUniquePredecessor();
+ return PrevBB && PrevBB->getUniqueSuccessor() ? PrevBB : nullptr;
+ };
----------------
As an idea for follow-up: if `PrevBB` is the only exit of a loop (w/o abnormal exits or locks etc), we can also return it, because the loop should be finite. Maybe makes sense to add a TODO.
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6698
+ // we find a better cache update mechanism.
+ for (unsigned i = 1; i < Path.size() - 1; i++)
+ if (!LI.getLoopFor(Path[i]))
----------------
just an idea: merge the two loops together?
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6718
+ // we'd need to know where in the block the blockage is.
+ if (BlockTransferExecutionToSuccessorCache.count(BB) &&
+ BlockTransferExecutionToSuccessorCache[BB])
----------------
Can we use `lookup`?
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7665
+ for (auto *BB : CurrL->getBlocks())
+ BlockTransferExecutionToSuccessorCache.erase(BB);
LoopPropertiesCache.erase(CurrL);
----------------
What still worries me (though I can't say if there is a bug here) is that your path might contain blocks from multiple (nested) loops, and here we might only forget inner loop. Will that still be correct?
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6670
- auto *BLoop = LI.getLoopFor(B->getParent());
- if (BLoop && BLoop->getHeader() == B->getParent() &&
- BLoop->getLoopPreheader() == A->getParent() &&
- ::isGuaranteedToTransferExecutionToSuccessor(A->getIterator(),
- A->getParent()->end()) &&
- ::isGuaranteedToTransferExecutionToSuccessor(B->getParent()->begin(),
- B->getIterator()))
- return true;
- return false;
+ // First find a path from B to A where if all blocks along path
+ // are transparent, then we can prove A reaches B. Defer the actual
----------------
reames wrote:
> mkazantsev wrote:
> > Quick check: `A` dominates `B`?
> Two answers:
> As actually used, that's trivially true.
>
> For the interface, we don't need this property. Consider B in the header of some loop L, with A in the (unconditional) latch block. A must reach B even though A does not dominate B.
>
> This could be an optimization since the case implemented doesn't catch the case just mentioned. Will add with that as a comment.
Fair, I didn't think of this case. Unconditional latches are rare, though, so I think this is fine.
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6716
+ // local search, we use the block local cache as a filter.
+ auto doLocalSearch = [&](BasicBlock::const_iterator Begin,
+ BasicBlock::const_iterator End) {
----------------
reames wrote:
> mkazantsev wrote:
> > nit: capitalize lambda name.
> They're methods, not variables. (Or at least, I believe that's our style convention.)
I was certain they are treated as variables, and there is a lot of examples of this in this very file (grep by "= [", most are capitalized).
```
./ScalarEvolution.cpp:639: const auto IsGVNameSemantic = [&](const GlobalValue *GV) {
./ScalarEvolution.cpp:854: auto IsLessComplex = [&](const SCEV *LHS, const SCEV *RHS) {
./ScalarEvolution.cpp:2356: auto IsKnownNonNegative = [&](const SCEV *S) {
./ScalarEvolution.cpp:2370: auto Opcode = [&] {
./ScalarEvolution.cpp:2469: auto ComputeFlags = [this, OrigFlags](const ArrayRef<const SCEV *> Ops) {
./ScalarEvolution.cpp:2513: auto FindTruncSrcType = [&]() -> Type * {
./ScalarEvolution.cpp:3058: auto ComputeFlags = [this, OrigFlags](const ArrayRef<const SCEV *> Ops) {
./ScalarEvolution.cpp:3638: const bool AssumeInBoundsFlags = [&]() {
./ScalarEvolution.cpp:3752: auto FoldOp = [&](const APInt &LHS, const APInt &RHS) {
./ScalarEvolution.cpp:4196: auto MatchMinMaxNegation = [&](const SCEVMinMaxExpr *MME) {
./ScalarEvolution.cpp:5193: auto getExtendedExpr = [&](const SCEV *Expr,
./ScalarEvolution.cpp:5208: auto PredIsKnownFalse = [&](const SCEV *Expr,
./ScalarEvolution.cpp:5228: auto AppendPredicate = [&](const SCEV *Expr,
./ScalarEvolution.cpp:5300: auto areExprsEqual = [&](const SCEV *Expr1, const SCEV *Expr2) -> bool {
./ScalarEvolution.cpp:5738: auto CoerceOperand = [&](const SCEV *Op) -> const SCEV * {
./ScalarEvolution.cpp:6626: auto pushOp = [&](const SCEV *S) {
./ScalarEvolution.cpp:6771: auto HasSideEffects = [](Instruction *I) {
./ScalarEvolution.cpp:7692: auto PredicateNotAlwaysTrue = [](const ExitNotTakenInfo &ENT) {
./ScalarEvolution.cpp:7715: auto PredicateNotAlwaysTrue = [](const ExitNotTakenInfo &ENT) {
./ScalarEvolution.cpp:9404: auto SolveForBoundary = [&](APInt Bound) -> std::pair<Optional<APInt>,bool> {
./ScalarEvolution.cpp:9422: auto LeavesRange = [&] (const APInt &X) {
./ScalarEvolution.cpp:9688: auto ComputesEqualValues = [](const Instruction *A, const Instruction *B) {
./ScalarEvolution.cpp:9714: auto TrivialCase = [&](bool TriviallyTrue) {
./ScalarEvolution.cpp:10230: auto CheckRanges = [&](const ConstantRange &RangeLHS,
./ScalarEvolution.cpp:10261: auto MatchBinaryAddToConst = [this](const SCEV *X, const SCEV *Y,
./ScalarEvolution.cpp:10529: auto ProofFn = [&](ICmpInst::Predicate P) {
./ScalarEvolution.cpp:10537: auto ProveViaGuard = [&](const BasicBlock *Block) {
./ScalarEvolution.cpp:10541: auto ProofFn = [&](ICmpInst::Predicate P) {
./ScalarEvolution.cpp:10551: auto ProveViaCond = [&](const Value *Condition, bool Inverse) {
./ScalarEvolution.cpp:10556: auto ProofFn = [&](ICmpInst::Predicate P) {
./ScalarEvolution.cpp:10790: auto IsSignFlippedPredicate = [](CmpInst::Predicate P1,
./ScalarEvolution.cpp:11143: auto ProvedEasily = [&](const SCEV *S1, const SCEV *S2) {
./ScalarEvolution.cpp:11337: auto GetOpFromSExt = [&](const SCEV *S) {
./ScalarEvolution.cpp:11352: auto IsSGTViaContext = [&](const SCEV *S1, const SCEV *S2) {
./ScalarEvolution.cpp:11376: auto IsSumGreaterThanRHS = [&](const SCEV *S1, const SCEV *S2) {
./ScalarEvolution.cpp:11696: auto canAssumeNoSelfWrap = [&](const SCEVAddRecExpr *AR) {
./ScalarEvolution.cpp:11842: auto wouldZeroStrideBeUB = [&]() {
./ScalarEvolution.cpp:11859: auto isUBOnWrap = [&]() {
./ScalarEvolution.cpp:11954: auto canProveRHSGreaterThanEqualStart = [&]() {
./ScalarEvolution.cpp:12007: bool MayAddOverflow = [&] {
./ScalarEvolution.cpp:13581: const auto MatchURemWithDivisor = [&](const SCEV *B) {
./ScalarEvolution.cpp:13653: auto CollectCondition = [&](ICmpInst::Predicate Predicate, const SCEV *LHS,
```
================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6847
for (auto &I : *BB) {
- if (!isGuaranteedToTransferExecutionToSuccessor(&I))
- LP.HasNoAbnormalExits = false;
+ if (!llvm::isGuaranteedToTransferExecutionToSuccessor(&I))
+ Local.HasNoAbnormalExits = false;
----------------
reames wrote:
> mkazantsev wrote:
> > Does it handle `invoke` terminator correctly?
> Er, it's calling the same function as previously? Not sure what you mean?
I thought there might have been a bug before your patch, but it seems that `isGuaranteedToTransferExecutionToSuccessor` handles it correctly.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111353/new/
https://reviews.llvm.org/D111353
More information about the llvm-commits
mailing list