[PATCH] D42417: [SCEV] Fix isLoopEntryGuardedByCond

Max Kazantsev via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 2 00:56:52 PST 2018


mkazantsev added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:9066
+  // Both LHS and RHS must be available at loop entry.
+  if (!isAvailableAtLoopEntry(LHS, L) || !isAvailableAtLoopEntry(RHS, L))
+    return false;
----------------
mkazantsev wrote:
> sanjoy wrote:
> > skatkov wrote:
> > > sanjoy wrote:
> > > > I think this should be an assert -- if a caller is passing in unavailable SCEVs then it probably has other bugs too which this change will hide.
> > > Hi Sanjoy, I did first time this as an assert, see https://reviews.llvm.org/rL323077.
> > > And I revert it due to there were a lot of buildbot failures. Adding this check to every case makes the code is not readable :)
> > > 
> > > What do you think? Does it makes sense to do it as an assert and update all invocations in spite of readability?
> > > And I revert it due to there were a lot of buildbot failures. 
> > 
> > Can you give some examples of where this failed (i.e. from where are we passing in bogus LHS and RHS to isLoopEntryGuardedByCond)?  As I said, I won't be surprised if those places are buggy due to other reasons too.
> > 
> > I would be fine if you added a helper function that does the availability check and then calls `isLoopEntryGuardedByCond` but I suspect a descriptive name of the helper will be more annoying to type out than the availability check + the call to `isLoopEntryGuardedByCond`.
> I see the asserts in the initial patch were before we check `L` on `nullptr`. I guess this is the reason why buildbots failed. I also think that these should be asserts, and in exactly that place where they are now.
Also the initial patch lacks availability check in `isImpliedViaNoOverflow`. It was a real bug, I believe.


https://reviews.llvm.org/D42417





More information about the llvm-commits mailing list