[PATCH] D99987: [NewGVN] Track simplification dependencies for phi-of-ops.

[Ruiling, Song via Phabricator via llvm-commits]

ruiling added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/NewGVN.cpp:1874-1900
           if (CmpInst::isImpliedTrueByMatchingCmp(BranchPredicate,
                                                   OurPredicate)) {
             addPredicateUsers(PI, I);
             return createConstantExpression(
                 ConstantInt::getTrue(CI->getType()));
           }
 
----------------
I think we also need to return the PI->Condition as ExtraDep together with the expression for such cases, this is what I saw in the case PR49873 that we failed to mark the 'real' instruction (i.e. op-of-phi) depends on this predicate compare.


================
Comment at: llvm/lib/Transforms/Scalar/NewGVN.cpp:1908
+  auto Res = createExpression(I);
+  Res.addExtraUserTo([this](Value *V, Value *U) { addAdditionalUsers(V, U); },
+                     I);
----------------
Do we need to check that `I` is not a temp instruction before making it depends on other instruction? I think the `I` here may be a temp instruction if this is called from makePossiblePHIOfOps()->FindLeaderForInst()->performSymbolicEvaluation()->performSymbolicCmpEvaluation().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99987