[PATCH] D85046: [SCEV] If Start >= RHS, simplify (Start smin RHS) to RHS for trip counts (PR46924, PR46939)

[Max Kazantsev via Phabricator via llvm-commits]

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

LGTM. Good catch! Need to think about API to make this more generic.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10633
+    if (isLoopEntryGuardedByCond(
+            L, IsSigned ? ICmpInst::ICMP_SGE : ICmpInst::ICMP_UGE, Start, RHS))
+      End = RHS;
----------------
Please add comment like
```
// If we know that Start >= RHS in the context of loop, then we know that min(RHS, Start) = RHS
// at this point.
```


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10636
+    else
+      End = IsSigned ? getSMinExpr(RHS, Start) : getUMinExpr(RHS, Start);
+  }
----------------
mkazantsev wrote:
> If loop is guarded by condition `Start >= RHS`, then `getMinExpr(RHS, Start)` should be able to simplify to `RHS`. Seems that we are missing some simplifications in there. Can we improve the simplification instead of manually handling this particular case here?
Oh, I see what happens. We only know this fact in this point, and `getMinExpr` has no mechanism to figure out in which context it happens. Then it makes sense.

Maybe in the future we will want to have something like "get<SCEVType>Expr(Ops, Context)" for such cases. But here the fix seems crystal clear, so it should be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85046