<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, May 1, 2017 at 2:11 PM, Davide Italiano via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">davide created this revision.<br>
Herald added a subscriber: Prazek.<br>
<br>
The problem is here (debug output):<br>
<br>
  Processing instruction   %tmp4 = icmp sgt i32 %tmp.0, 0<br>
  Created new congruence class for   %tmp4 = icmp sgt i32 %tmp.0, 0 using expression { ExpressionTypeConstant, opcode = 18,  constant = i1 true} at 4 and leader i1 true<br>
  New class 4 for expression { ExpressionTypeConstant, opcode = 18,  constant = i1 true}<br>
<br>
we believe %tmp1 implies %tmp4<br>
<br>
where:<br>
<br>
  br i1 %tmp1, label %bb2, label %bb7<br>
  br i1 %tmp4, label %bb5, label %bb7<br>
<br>
because while looking at PredicateInfo stuffs we end up calling isImpliedTrueByMatchingCmp() with the operands swapped,<br>
`y less then x` implies `y less then or equal x` but the other way around is not true.<br></blockquote><div><br></div><div>But we also swap the branch predicate when we swap operands.<br></div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The function has a very confusing name IMHO. I expect `isImpliedTrueByMatchingCmp` means that *the first argument* is implied by the second and not viceversa. I'm willing to change it, but I wonder if we have other latent bugs in tree because of this.<br></blockquote><div><br></div><div>Oh you mean, literally, the isImplied function is being used backwards.</div><div><br></div><div>Sigh.</div><div><br></div><div>LGTM </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D32718" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D32718</a><br>
<br>
Files:<br>
  lib/Transforms/Scalar/NewGVN.<wbr>cpp<br>
  test/Transforms/NewGVN/<wbr>pr32852.ll<br>
<br>
<br>
Index: test/Transforms/NewGVN/<wbr>pr32852.ll<br>
==============================<wbr>==============================<wbr>=======<br>
--- /dev/null<br>
+++ test/Transforms/NewGVN/<wbr>pr32852.ll<br>
@@ -0,0 +1,24 @@<br>
+; Make sure GVN doesn't incorrectly think the branch terminating<br>
+; bb2 has a constant condition.<br>
+; RUN: opt -S -newgvn %s | FileCheck %s<br>
+<br>
+@a = common global i32 0<br>
+@.str = private unnamed_addr constant [3 x i8] c"0\0A\00"<br>
+<br>
+define void @main() {<br>
+bb:<br>
+  %tmp = load i32, i32* @a<br>
+  %tmp1 = icmp sge i32 %tmp, 0<br>
+  br i1 %tmp1, label %bb2, label %bb7<br>
+bb2:<br>
+  %tmp4 = icmp sgt i32 %tmp, 0<br>
+; CHECK: br i1 %tmp4, label %bb5, label %bb7<br>
+  br i1 %tmp4, label %bb5, label %bb7<br>
+bb5:<br>
+  %tmp6 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @.str, i32 0, i32 0))<br>
+  br label %bb7<br>
+bb7:<br>
+  ret void<br>
+}<br>
+<br>
+declare i32 @printf(i8*, ...)<br>
Index: lib/Transforms/Scalar/NewGVN.<wbr>cpp<br>
==============================<wbr>==============================<wbr>=======<br>
--- lib/Transforms/Scalar/NewGVN.<wbr>cpp<br>
+++ lib/Transforms/Scalar/NewGVN.<wbr>cpp<br>
@@ -1628,15 +1628,15 @@<br>
         if (PBranch->TrueEdge) {<br>
           // If we know the previous predicate is true and we are in the true<br>
           // edge then we may be implied true or false.<br>
-          if (CmpInst::<wbr>isImpliedTrueByMatchingCmp(<wbr>OurPredicate,<br>
-                                                  BranchPredicate)) {<br>
+          if (CmpInst::<wbr>isImpliedTrueByMatchingCmp(<wbr>BranchPredicate,<br>
+                                                  OurPredicate)) {<br>
             addPredicateUsers(PI, I);<br>
             return createConstantExpression(<br>
                 ConstantInt::getTrue(CI-><wbr>getType()));<br>
           }<br>
<br>
-          if (CmpInst::<wbr>isImpliedFalseByMatchingCmp(<wbr>OurPredicate,<br>
-                                                   BranchPredicate)) {<br>
+          if (CmpInst::<wbr>isImpliedFalseByMatchingCmp(<wbr>BranchPredicate,<br>
+                                                   OurPredicate)) {<br>
             addPredicateUsers(PI, I);<br>
             return createConstantExpression(<br>
                 ConstantInt::getFalse(CI-><wbr>getType()));<br>
<br>
<br>
</blockquote></div><br></div></div>