[PATCH] D42417: [SCEV] Fix isLoopEntryGuardedByCond

[Max Kazantsev via Phabricator via llvm-commits]

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


https://reviews.llvm.org/D42417