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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for taking a look! I'll add the comment as suggested in the commit. I'm more than happy to collaborate/brainstorm/work on a way to get context-specific version of SCEV expressions as follow-up.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10636
+    else
+      End = IsSigned ? getSMinExpr(RHS, Start) : getUMinExpr(RHS, Start);
+  }
----------------
mkazantsev wrote:
> 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.
> 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.

Yep, that's specifically in the context of the loop where this information is available. I added the comment you suggested above, this should make this much clearer.

> 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.

Something like that would indeed be quite useful in some cases I think. IIRC getting an expression for a specific context could also help with retaining wrapping flags in some cases. I would be more than happy to collaborate/work on a more general solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85046