[PATCH] D42417: [SCEV] Fix isLoopEntryGuardedByCond

[Sanjoy Das via Phabricator via llvm-commits]

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


https://reviews.llvm.org/D42417