[PATCH] D15559: [SCEVExpander] Make findExistingExpansion smarter

[Michael Zolotukhin via llvm-commits]

mzolotukhin added inline comments.

================
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;
----------------
flyingforyou wrote:
> mzolotukhin wrote:
> > 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?
> I think LHS can be Value. But what I know is above pattern can cover most of cases.
> 
> 
> ```
>     if (match(BB->getTerminator(),
>                m_Br(m_ICmp(Pred, m_Value(LHSV), m_Value(RHSV)), TrueBB,
>                     FalseBB))) {
>       dbgs() << "m_Value(LHSV), m_Value(RHSV) pattern\n";
>       if (match(BB->getTerminator(),
>                  m_Br(m_ICmp(Pred, m_Instruction(LHS), m_Instruction(RHS)), TrueBB,
>                       FalseBB)))
>         dbgs() << "m_Instruction(LHS), m_Instruction(RHS) pattern\n";
>       else if (match(BB->getTerminator(),
>                  m_Br(m_ICmp(Pred, m_Instruction(LHS), m_Value(RHSV)), TrueBB,
>                       FalseBB)))
>         dbgs() << "m_Instruction(LHS), m_Value(RHSV) pattern\n";
>       else
>         dbgs() << "unknown(LHS), unknown(RHS) pattern\n";
> 
>     }
> ```
> 
> When I test above code, I can't find "unknown(LHS), unknown(RHS) pattern\n" on building big benchmarks programs including LLVM itself.
> 
> 
> 
> |m_Value(LHSV), m_Value(RHSV) pattern |50650
> |m_Instruction(LHS), m_Instruction(RHS) pattern|30178
> |m_Instruction(LHS), m_Value(RHSV) pattern |20472
> 
> So I think it's no necessary to consider `LHS `is `Value` now.
> 
> Actually, we don't make compare instruction likes `cmp constant, reg`...
My concern isn't about us missing some real cases right now, it's more about future maintenance of this code. If later on someone finds such a case, it'll be hard to figure out, why isn't it covered in the first place, when the code was committed.

================
Comment at: test/Transforms/LoopUnroll/high-cost-trip-count-computation.ll:53
@@ -27,1 +52,2 @@
+
 !0 = !{i64 1, i64 100}
----------------
flyingforyou wrote:
> mzolotukhin wrote:
> > This metadata is used as `!range !0` in `@test` and as `!tbaa !0` in `@test2`. Are you sure this is correct?
> I don't think that makes errors. But I got your point. Thanks.
> I will modify `!tbaa !0 ` to `!tbaa !1`
> and add `!1 = !{i64 0}`
> 
> Is this ok?
This is ok (or you can just remove `!tbaa !1` and `!1 = !{i64 0}` at all). Thanks!


http://reviews.llvm.org/D15559