[ValueTracking] Cleanup condition for dup incoming edge for phi nodes; NFC

[nikic]

LG

[goldstein.w.n]

Rebase

[goldstein.w.n]

This isn't NFC. It breaks some tests in clang. Going to drop it.

[nikic]

Can you extract the failing clang test into an LLVM test? I don't understand why this isn't NFC.

[goldstein.w.n]

In D157816#4590970, @nikic wrote:

Can you extract the failing clang test into an LLVM test? I don't understand why this isn't NFC.

Clang :: CodeGenCXX/temp-order.cpp
Clang :: Headers/__clang_hip_math.hip

I looked into temp-order.cpp and its properly different.
All the print calls disappear.

[nikic]

It looks like the issue occurs during inlining. I expect what happens is that not the terminator hasn't been remapped yet and references blocks from the old function. This seems like something that is just waiting to blow up...

[nikic]

I've added a test in https://github.com/llvm/llvm-project/commit/6f3f600b2aa10df3b9de580e9fd9457b93e3d087.

[goldstein.w.n]

In D157816#4591108, @nikic wrote:

I've added a test in https://github.com/llvm/llvm-project/commit/6f3f600b2aa10df3b9de580e9fd9457b93e3d087.

Do we need any associated fix?

[goldstein.w.n]

In D157816#4591108, @nikic wrote:

I've added a test in https://github.com/llvm/llvm-project/commit/6f3f600b2aa10df3b9de580e9fd9457b93e3d087.

Confirmed, this change does break the test.

llvm/lib/Analysis/ValueTracking.cpp

if (Depth < MaxAnalysisRecursionDepth - 1 && Known.isUnknown()) {
           ICmpInst::Predicate Pred;
           const APInt *RHSC;
           BasicBlock *TrueSucc, *FalseSucc;
           // TODO: Use RHS Value and compute range from its known bits.
           if (match(RecQ.CxtI,
                     m_Br(m_c_ICmp(Pred, m_Specific(IncValue), m_APInt(RHSC)),
                          m_BasicBlock(TrueSucc), m_BasicBlock(FalseSucc)))) {
             // Check for cases of duplicate successors.
-            if ((TrueSucc == P->getParent()) != (FalseSucc == P->getParent())) {
+            if (TrueSucc != FalseSucc) {
               // If we're using the false successor, invert the predicate.
               if (FalseSucc == P->getParent())
                 Pred = CmpInst::getInversePredicate(Pred);
               // Get the knownbits implied by the incoming phi condition.
               auto CR = ConstantRange::makeExactICmpRegion(Pred, *RHSC);
               Known2 = CR.toKnownBits();
             }
           }
return llvm::all_of(PN->operands(), [&](const Use &U) {
       // Check if the branch on the phi excludes zero.
       ICmpInst::Predicate Pred;
       Value *X;
       BasicBlock *TrueSucc, *FalseSucc;
       if (match(RecQ.CxtI,
                 m_Br(m_c_ICmp(Pred, m_Specific(U.get()), m_Value(X)),
                      m_BasicBlock(TrueSucc), m_BasicBlock(FalseSucc)))) {
         // Check for cases of duplicate successors.
-        if ((TrueSucc == PN->getParent()) != (FalseSucc == PN->getParent())) {
+        if (TrueSucc != FalseSucc) {
           // If we're using the false successor, invert the predicate.
           if (FalseSucc == PN->getParent())
             Pred = CmpInst::getInversePredicate(Pred);
           if (cmpExcludesZero(Pred, X))
             return true;
         }
       }
       // Finally recurse on the edge and check it directly.