[PATCH] D32718: [NewGVN] Don't derive incorrect implications.

[Daniel Berlin via llvm-commits]

On Mon, May 1, 2017 at 2:11 PM, Davide Italiano via Phabricator <
reviews at reviews.llvm.org> wrote:

> davide created this revision.
> Herald added a subscriber: Prazek.
>
> The problem is here (debug output):
>
>   Processing instruction   %tmp4 = icmp sgt i32 %tmp.0, 0
>   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
>   New class 4 for expression { ExpressionTypeConstant, opcode = 18,
> constant = i1 true}
>
> we believe %tmp1 implies %tmp4
>
> where:
>
>   br i1 %tmp1, label %bb2, label %bb7
>   br i1 %tmp4, label %bb5, label %bb7
>
> because while looking at PredicateInfo stuffs we end up calling
> isImpliedTrueByMatchingCmp() with the operands swapped,
> `y less then x` implies `y less then or equal x` but the other way around
> is not true.
>

But we also swap the branch predicate when we swap operands.



>
> 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.
>

Oh you mean, literally, the isImplied function is being used backwards.

Sigh.

LGTM

>
>
> https://reviews.llvm.org/D32718
>
> Files:
>   lib/Transforms/Scalar/NewGVN.cpp
>   test/Transforms/NewGVN/pr32852.ll
>
>
> Index: test/Transforms/NewGVN/pr32852.ll
> ===================================================================
> --- /dev/null
> +++ test/Transforms/NewGVN/pr32852.ll
> @@ -0,0 +1,24 @@
> +; Make sure GVN doesn't incorrectly think the branch terminating
> +; bb2 has a constant condition.
> +; RUN: opt -S -newgvn %s | FileCheck %s
> +
> + at a = common global i32 0
> + at .str = private unnamed_addr constant [3 x i8] c"0\0A\00"
> +
> +define void @main() {
> +bb:
> +  %tmp = load i32, i32* @a
> +  %tmp1 = icmp sge i32 %tmp, 0
> +  br i1 %tmp1, label %bb2, label %bb7
> +bb2:
> +  %tmp4 = icmp sgt i32 %tmp, 0
> +; CHECK: br i1 %tmp4, label %bb5, label %bb7
> +  br i1 %tmp4, label %bb5, label %bb7
> +bb5:
> +  %tmp6 = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([3 x
> i8], [3 x i8]* @.str, i32 0, i32 0))
> +  br label %bb7
> +bb7:
> +  ret void
> +}
> +
> +declare i32 @printf(i8*, ...)
> Index: lib/Transforms/Scalar/NewGVN.cpp
> ===================================================================
> --- lib/Transforms/Scalar/NewGVN.cpp
> +++ lib/Transforms/Scalar/NewGVN.cpp
> @@ -1628,15 +1628,15 @@
>          if (PBranch->TrueEdge) {
>            // If we know the previous predicate is true and we are in the
> true
>            // edge then we may be implied true or false.
> -          if (CmpInst::isImpliedTrueByMatchingCmp(OurPredicate,
> -                                                  BranchPredicate)) {
> +          if (CmpInst::isImpliedTrueByMatchingCmp(BranchPredicate,
> +                                                  OurPredicate)) {
>              addPredicateUsers(PI, I);
>              return createConstantExpression(
>                  ConstantInt::getTrue(CI->getType()));
>            }
>
> -          if (CmpInst::isImpliedFalseByMatchingCmp(OurPredicate,
> -                                                   BranchPredicate)) {
> +          if (CmpInst::isImpliedFalseByMatchingCmp(BranchPredicate,
> +                                                   OurPredicate)) {
>              addPredicateUsers(PI, I);
>              return createConstantExpression(
>                  ConstantInt::getFalse(CI->getType()));
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170501/96eaa54a/attachment.html>