[PATCH] D33172: [InstCombine] Simpify inverted predicates in 'or'

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

In https://reviews.llvm.org/D33172#755069, @spatel wrote:

> I agree that we're missing some kind of more general transform. Eg:
>
>     int goo(int x, int y, int r) {
>       if (x > y) r += 42;
>       if (x <= y) r += 12;
>       return r;
>     }
>  
>   define i32 @goo(i32, i32, i32) {
>     %4 = icmp sgt i32 %0, %1
>     %5 = add nsw i32 %2, 42
>     %6 = select i1 %4, i32 %5, i32 %2
>     %7 = add nsw i32 %6, 12
>     %8 = select i1 %4, i32 %5, i32 %7
>     ret i32 %8
>   }
>
>
> https://godbolt.org/g/tszo72
>
> It's worth noting that one motivation for PR32791 (although I forgot to write it there) is that I think we should canonicalize all:
>  not (cmp P, X, Y) --> cmp P', X, Y
>  Ie, invert the predicate to eliminate an explicit 'not' (xor) because then we have eliminated a dependency in the IR. This was discussed in https://reviews.llvm.org/D32725.
>
> So we should look at how the existing InstCombine test ("@poo" is just above the new test here) fails if we implement that canonicalization:
>
>   define i32 @poo5(i32 %a, i32 %b, i32 %c, i32 %d) {
>     %cmp = icmp slt i32 %a, %b
>     %and1 = select i1 %cmp, i32 %c, i32 0
>     %cmpnot = xor i1 %cmp, -1   <--- this can be replaced by "icmp sge i32 %a, %b", and we have the first testcase in this patch (the test from PR32791)
>     %and2 = select i1 %cmpnot, i32 %d, i32 0
>     %or = or i32 %and1, %and2
>     ret i32 %or
>   }
>  
>   define i32 @poo6(i32 %a, i32 %b, i32 %c, i32 %d) {
>     %cmp = icmp slt i32 %a, %b
>     %and1 = select i1 %cmp, i32 %c, i32 0
>     %and2 = select i1 %cmp, i32 0, i32 %d  <--- we fold a 'not' compare by swapping the select operands
>     %or = or i32 %and1, %and2
>     ret i32 %or
>   }
>  
>
>
> For this pattern, there's a fold in InstCombiner::SimplifyUsingDistributiveLaws():
>
>   // (op (select (a, c, b)), (select (a, d, b))) -> (select (a, (op c, d), 0))
>   // (op (select (a, b, c)), (select (a, b, d))) -> (select (a, 0, (op c, d)))
>   
>
> Note: the implementation for that fold seems bogus. We create a new binop and a new select for *both* of the current selects in this example and then don't use one pair.
>
> So if we want a more general InstCombine solution than what is currently proposed here, I think we should enhance that to account for an inverted predicate as well as matching predicates:
>  binop (select P, A, B), (select P', C, D) --> binop (select P, A, B), (select P, D, C)
>  and let the existing fold take it from there.
>
> Swapping select operands to eliminate an inverted compare seems like a generally good fold for some other pass too. Would that be GVN?


Not entirely sure I follow this last one.
NewGVN finds whether two expressions e1 and e2 are equivalent (actually, not general equivalence as checking equivalence of program expressions is undecidable, so we approximate with Herbrand equivalence). If you have an example (IR), that will help (and I can take a look).


https://reviews.llvm.org/D33172