[PATCH] D139403: [SCEV] Compute symbolic exit count for 'and' conditions

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8715
+    if (EL.ExactNotTaken != getCouldNotCompute() ||
+        EL.SymbolicMaxNotTaken != getCouldNotCompute())
       ExitCounts.emplace_back(ExitBB, EL);
----------------
nikic wrote:
> Why the new condition here?
The old logic was: put to ExitCounts whenever an exact count is known. Then, when we request loop's exact exit count, we traverse through them and take umin.

Now in the test `symbolic_max_exit_count.ll` exact is not known. But we still want to put it into this vector to be able to request symbolic exit count.

That's why now we put it if either exact or symbolic is known. I think we can relax it to just symbolic (exact always implies it).


================
Comment at: llvm/test/Analysis/ScalarEvolution/symbolic_max_exit_count.ll:227
   ret i32 -3
 }
 
----------------
nikic wrote:
> I'm missing a test for the case where we actually produce a umin_seq.
In fact we have it in `test/Analysis/ScalarEvolution/exit-count-select-safe.ll`. This patch preserves current behavior, however if I remove `/*IsSequential*/ true`, it changes. Is it enough or you want a new one specifically for this?


================
Comment at: llvm/test/Analysis/ScalarEvolution/widenable-condition.ll:27
 ; CHECK-NEXT:  Loop %loop: constant max backedge-taken count is 1999
-; CHECK-NEXT:  Loop %loop: Unpredictable symbolic max backedge-taken count.
+; CHECK-NEXT:  Loop %loop: symbolic max backedge-taken count is 1999
 ; CHECK-NEXT:  Loop %loop: Unpredictable predicated backedge-taken count.
----------------
nikic wrote:
> I don't really get why this patch introduces these changes (i.e. why this wasn't the result previously). Aren't we already assigning the constant max to the symbol max if there is no exact?
This is indeed strange. I will investigate.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139403/new/

https://reviews.llvm.org/D139403