[PATCH] D148661: [SCEV] Common code for computing trip count in a fixed type [NFC-ish]

[Joshua Cao via Phabricator via llvm-commits]

caojoshua added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8051
+  // Get the total trip count from the count by adding 1.
+  return getAddExpr(getZeroExtendExpr(ExitCount, EvalTy), getOne(EvalTy));
+}
----------------
Does this break existing calls to `getTripCountFromExitCount()`? Now all trip counts will be ZeroExtended.

Although this is not breaking any current tests, I do have https://reviews.llvm.org/D141823 open which depends on removing the zext when possible when computing trip multiples.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8069
+
+    if (L && isLoopEntryGuardedByCond(L, ICmpInst::ICMP_NE, ExitCount,
+                                      getMinusOne(ExitCount->getType())))
----------------
no need for `if` here
```
return L && isLoopEntryGuardedByCond...
```

Also, not sure if its better, but I think computing `applyLoopGuards(ExitCount, L)` and then checking the RangeRef of the resulting SCEV will supersede `isLoopEntryGuardedByCond` and probably cover more other cases.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8079
+  // safe, it allows better simplification of the +1.
+  if (EvalSize > ExitCountSize && CanAddOneWithoutOverflow()) {
+    return getZeroExtendExpr(
----------------
unnecessary brackets

I believe this should be `!CanAddOneWithoutOverflow()`. If its true, then there is no reason to zext.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:994
+  const SCEV *TripCountSCEV =
+    SE->getTripCountFromExitCount(BECount, IntPtr, CurLoop);
   return SE->getMulExpr(TripCountSCEV,
----------------
craig.topper wrote:
> Isn't clang-format for line wrap 4 spaces of indention?
I think 2 spaces is correct. If I run 

```
clang-format -style=llvm -dump-config > .clang-format
```

I see `IndentWidth: 2` and `UseTab: Never`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148661