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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin marked 3 inline comments as done.
dberlin added inline comments.


================
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);
----------------
davide wrote:
> 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.
Yeah, i think, over time, we should try to move a lot of this logic to some central place.

Simplify* knows how to do a *lot* of it (and sadly, does it's own walking to try to figure out some cases), but it's a complex maze of twisty passages, and it's not clear to me how to shove predicate info into it in a sane way.

Most of these tests are covered by edge.ll, which we'll pass once the switch stuff goes in


================
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()) {
----------------
davide wrote:
> s/Things/intrinsics/  (or instructions)? Also, full stop at the end of the comment I think (nit)
Fixed.


================
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()));
----------------
davide wrote:
> I'm not sure this is entirely right, do you have a test where this triggers so I can take a look?
This is from GVN:

02244       // If "A >= B" is known true, replace "A < B" with false everywhere.
02245       CmpInst::Predicate NotPred = Cmp->getInversePredicate();
02246       Constant *NotVal = ConstantInt::get(Cmp->getType(), isKnownFalse);

It does not restrict itself to equality predicates.

I'll grab the appropriate testcases, but the only cases i believe this could be false is somewhere in the land of floating point.

I also thought it was weird, but:

Looking at it from the perspective of truth tables, inverse predicate just returns a truth table that has true and false swapped.

So if the other comparison is false, and it has the same operands, the inverse predicate must be true (because every place that predicate has false in the truth table, this one has true)





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


https://reviews.llvm.org/D29682