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

Wed May 31 21:04:07 PDT 2023

caojoshua marked an inline comment as done.
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:
> 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.
Issue fixed by https://github.com/llvm/llvm-project/commit/ff471dcf7669b1ad7903a44d0773bef4eb175eb9

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149529