[PATCH] D62880: Prepare for multi-exit LFTR [NFC]

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

> I should just include the resultng test change in this commit and not worry about it.

I would say don't worry about it.  This should be rare, but not unheard of.  E.g. there are extreme cases where SCEV will be able to constant for A-B but not B-A (where A and B are complex expressions).



================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2201
       // increase the number of undef users.
-      if (ICmpInst *Cond = getLoopTest(L)) {
-        if (Phi != getLoopPhiForCounter(Cond->getOperand(0), L) &&
-            Phi != getLoopPhiForCounter(Cond->getOperand(1), L)) {
-          continue;
-        }
-      }
+      // TODO: Generlize this to allow *any* loop exit which is known to
+      // execute on each iteration
----------------
Generalize


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2203
+      // execute on each iteration
+      if (L->getExitingBlock())
+        if (ICmpInst *Cond = getLoopTest(L, ExitingBB))
----------------
It would probably be more obvious if you did

```
bool HasOnlyOneExitingBlock = L->getExitingBlock() != nullptr;
if (HasOnlyOneExitingBlock) {
  ...
}
```

or something like that.

But isn't this called only when the loop has one exiting block (so the check will always return true)?


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2650
+      // choice; this was done to keep a change NFC.
+      const SCEV *BETakenCount = L->getExitingBlock() ?
+        BackedgeTakenCount : SE->getExitCount(L, ExitingBB);
----------------
I think this comment can be clearer about how it ties to the code.  Are you saying asking SCEV to recompute `getBackedgeTakenCount` here can produce different results so you take care to "re-use" `BackedgeTakenCount`?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62880/new/

https://reviews.llvm.org/D62880