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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

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?


https://reviews.llvm.org/D33172