[PATCH] D149529: [SCEV][reland] More precise trip multiples

[Joshua Cao via Phabricator via llvm-commits]

caojoshua added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:14326
+         (Multiple == 0 || RecomputedMultiple == 0)) &&
+        RecomputedMultiple.urem(Multiple) != 0) {
+      dbgs() << "Incorrect cached computation in ConstantMultipleCache for "
----------------
caojoshua wrote:
> uabelho wrote:
> > It's this urem call that crashes in the comment I made yesterday.
> > 
> > Multiple is 0 and doing urem with RHS being 0 hits the assertion since dividing by  0 isn't good.
> > 
> > Are we perhaps missing a negation of the condition
> > ```
> > (Multiple == 0 || RecomputedMultiple == 0)
> > ```
> > ?
> > Now we do the urem(Multiple) specifically if Multiple is 0, which we should avoid.
> Its due to returning a zero from [too many trailing zeros](https://github.com/llvm/llvm-project/blob/b7e5cb1f9a3a5226f22bb81c865214be81dce940/llvm/lib/Analysis/ScalarEvolution.cpp#L6233). I am going to rewrite this a bit, it should be verifying on `getNonZeroConstantMultiple()`.
> 
> I am going to run this through expensive checks.
Correction: I looked at things wrong. The current verification requires that recomputed multiples are stronger than previous multiples, but that is not the case here. It turns out a multiple can become weaker if due to dependence on ComputeKnownBits(). As the IR transforms, its possible that ComputeKnownBits() becomes weaker due to limitations in depth.

I have local changes that relaxes the verification and passes the provided test case. Testing with expensive checks takes a long time on my machine and I won't be able to push today.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149529