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

[Florian Hahn via Phabricator via llvm-commits]

fhahn 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()));
           }
 
----------------
ruiling wrote:
> 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.
Thanks, I'll give that a try, but I think it's worth to do this as follow-up patch.


================
Comment at: llvm/lib/Transforms/Scalar/NewGVN.cpp:1908
+  auto Res = createExpression(I);
+  Res.addExtraUserTo([this](Value *V, Value *U) { addAdditionalUsers(V, U); },
+                     I);
----------------
ruiling wrote:
> 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().
That's a good point, thanks! I updated performSymbolicCmpEvaluation to directly return `ExprResult` as well, so this case should also be handled properly in the caller now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99987