[PATCH] D31238: [ScalarEvolution] Re-enable Predicate implication from operations

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:8549
+                                             unsigned Depth) {
+  // We want to avoid hurting the compile time with analysis of too big trees.
+  if (Depth > MaxSCEVOperationsImplicationDepth)
----------------
sanjoy wrote:
> How about asserting here that
> 
>  - The types of `LHS` and `RHS` match
>  - The types of `FoundLHS` and `FoundRHS` match
> 
> ?
This assertion actually fails on test Transforms/IndVarSimplify/2011-11-01-lftrptr.ll even if placed to the beginning of isImpliedCondOperandsHelper method (and ViaOperations stuff is disabled). I am not certain for 100% is it a bug or not, but potentially it is. Even if so, its reason lies outside this patch. I will try to investigate it.

Currently we do not require the match of these types for our code here (yet I assumed that they do match). All required type conversions within isImpliedViaOperations are done explicitly. So let us not do it here. Instead, I will try to understand whether the failure on this assertion on those test is a bug, and if it is, I will add them with the fix patch.


https://reviews.llvm.org/D31238