[PATCH] D15559: [SCEVExpander] Make findExistingExpansion smarter

[Michael Zolotukhin via llvm-commits]

mzolotukhin requested changes to this revision.
mzolotukhin added a comment.
This revision now requires changes to proceed.

Hi Junmo,

Please find some comments inline.

Thanks,
Michael


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1863-1864
@@ -1838,4 +1862,4 @@
     if (!match(BB->getTerminator(),
-               m_Br(m_ICmp(Pred, m_Instruction(LHS), m_Instruction(RHS)),
-                    TrueBB, FalseBB)))
+               m_Br(m_ICmp(Pred, m_Instruction(LHS), m_Value(RHSV)), TrueBB,
+                    FalseBB)))
       continue;
----------------
I don't understand why we need to break symmetry in this code. Anyway, if we do this, we should either add an assert that the opposite situation never happens, or properly handle both cases. The fact that we've only seen one case is not a justification for not handling the other one.

IOW, why can't LHS be Value, and RHS be Instruction?

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:346-347
@@ -343,4 +345,4 @@
   //  extra iterations = run-time trip count % (loop unroll factor + 1)
   Value *TripCount = Expander.expandCodeFor(TripCountSC, TripCountSC->getType(),
                                             PreHeaderBR);
   Value *BECount = Expander.expandCodeFor(BECountSC, BECountSC->getType(),
----------------
This should be in the same patch, otherwise the change will only paper over the issue, and lead to actually worse code in the end. If `expandCodeFor` isn't updated, it'll fail to reuse the existing value, so it'll generate yet another division. The idea of the patch is to teach SCEV to reuse existing expressions, not just hack around the high cost of division. The division should still be considered expensive, so we definitely shouldn't generate redundant ones.


================
Comment at: test/Transforms/LoopUnroll/high-cost-trip-count-computation.ll:27-28
@@ -26,1 +26,4 @@
 
+;; We expect this loop to be unrolled, because IV's step is minus one.
+;; In this case, we don't need to generate dvision for computing trip count.
+
----------------
Maybe something like this?
```
Though SCEV for loop tripcount contains division, it shouldn't be considered expensive, since the division already exists in the code and we don't need to expand it once more. Thus, it shouldn't prevent us from unrolling the loop.
```
Also, I think I don't completely understand why we care so much about step being -1. Why is it important?

================
Comment at: test/Transforms/LoopUnroll/high-cost-trip-count-computation.ll:53
@@ -27,1 +52,2 @@
+
 !0 = !{i64 1, i64 100}
----------------
This metadata is used as `!range !0` in `@test` and as `!tbaa !0` in `@test2`. Are you sure this is correct?


http://reviews.llvm.org/D15559