[PATCH] D29682: NewGVN: Start making use of predicateinfo pass.

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

THis needs some tests (unless there are some which are are `XFAIL'D` and now pass). Other than that, great to see this making progress =)
Some comments inline.



================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:867-895
+  if (isa<PredicateAssume>(PI)) {
+    // If the comparison is true when the operands are equal, then we know the
+    // operands are equal, because assumes must always be true.
+    if (CmpInst::isTrueWhenEqual(Predicate))
+      if (auto *C = dyn_cast<Constant>(FirstOp))
+        return createConstantExpression(C);
+    return createVariableExpression(FirstOp);
----------------
I starred at this for long, and I think it's correct. I do also think you can simplify this to make it *slightly* more readable, at least in my opinion, removing the `else` after return.


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:903
+  if (auto *II = dyn_cast<IntrinsicInst>(I)) {
+    // Things with the returned attribute are copies of arguments
+    if (auto *ReturnedValue = II->getReturnedArgOperand()) {
----------------
s/Things/intrinsics/  (or instructions)? Also, full stop at the end of the comment I think (nit)


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1110-1113
+            // Inverse predicate, we know the other was false, so this is true.
+            // FIXME: Double check this
+            return createConstantExpression(
+                ConstantInt::getTrue(CI->getType()));
----------------
I'm not sure this is entirely right, do you have a test where this triggers so I can take a look?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:1841
   MSSA = _MSSA;
+  PredInfo = new PredicateInfo(F, *DT, *AC);
   DL = &F.getParent()->getDataLayout();
----------------
Any chance you can use something like `unique_ptr<>` instead of naked `new` (or something similar)?


================
Comment at: lib/Transforms/Scalar/NewGVN.cpp:2480-2481
+          if (auto *ReplacedInst = dyn_cast<Instruction>(MemberUse->get())) {
+            // Skip this if we are replacing predicateinfo with it's original
+            // operand, as we already know we can just drop it.
+            auto *PI = PredInfo->getPredicateInfoFor(ReplacedInst);
----------------
s/it's/its/


https://reviews.llvm.org/D29682