[PATCH] D44001: [SCEV] Prove implications for SCEVUnknown Phis

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

In https://reviews.llvm.org/D44001#1050287, @mkazantsev wrote:

> Actually ignoring of unreachable predecessors was supposed to be an improvement, but we don't really need it. We can yield it for sake of compile time saving.


Could you add a test case for unreachable predecessors? They are dominated by everything, so they would just pass through the checks below. I guess the expected improvement was to avoid cases where the unreachable blocks failed `ProvedEasily` check... A test case shows "unreachable blocks" go through the code, we just never handle them separately.



================
Comment at: lib/Analysis/ScalarEvolution.cpp:9585
+        continue;
+      assert(dominates(LHS, IncBB) && "Dominance broken?");
+      const SCEV *R = getSCEV(RPhi->getIncomingValueForBlock(IncBB));
----------------
mkazantsev wrote:
> This may fail if LHS is also a Phi from the same BB, but not a SCEVUnknown. It seems that it should be a check instead of assert.
I don't get this comment. We already finished checking that LHS is a phi in the above 2 cases in if-else ladder.


https://reviews.llvm.org/D44001