[PATCH] D105216: [ScalarEvolution] Fix overflow in computeBECount.

Tue Jul 13 10:06:09 PDT 2021

reames added a comment.

In D105216#2872827 <https://reviews.llvm.org/D105216#2872827>, @efriedma wrote:

>> but I *strongly* request you look for ways to factor the code so that the reasoning is modular
>
> I really don't see any way to unify the two different proof strategies.  The proofs are trying to reason about overflow on completely different operations.

So, I think this might be part of our problem.  The code and comments to date have not made your last sentence here obvious to me as the reviewer.

To be clear, I have not seen a concise description of what the bug in the original patch *was*.  That makes it hard to review a patch which is supposed to fix it.

>> If we can split this into two or more patches, that would be my strong preference.
>
> I can split off the introduction of getUDivCeilSCEV() into a separate patch, and I can introduce the new `isLoopEntryGuardedByCond(L, CondGT, Start, StartMinusStride)` check in a separate patch.  I'm not sure the result is easier to review, but I guess it would make it easier to figure out regressions.

Splitting patches to isolate regressions is good!

I went ahead and landed e4b43973 <https://reviews.llvm.org/rGe4b43973fbd41aee3b8197cf250e9fb9ac40f986>.  This handles only the constant bounds cases using the new ceiling operation.  This is an easy sub-case because a) stride is known positive, b) it'll constant fold thus not changing the result at all, and c) it doesn't appear to be influenced by your most recent proof.  If you rebase over that, it should help a bit to reduce complexity.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105216