[X86] Unsigned saturation subtraction canonicalization [the backend part]

[yulia_koval]

Summary

This is the backend part of unsigned saturation canonicalization patch. https://reviews.llvm.org/D37510

The patch transforms canonical version of unsigned saturation, which is sub(max(a,b),a) or sub(a,min(a,b)) to special psubus insturuction on targets, which support it(8bit and 16bit uints).
umax(a,b) - b -> subus(a,b)
a - umin(a,b) -> subus(a,b)

There is also extra case handled, when right part of sub is 32 bit and can be truncated, using UMIN(this transformation was discussed in https://reviews.llvm.org/D25987).

The example of special case code:

void foo(unsigned short *p, int max, int n) {

  int i;
  unsigned m;
  for (i = 0; i < n; i++) {
    m = *--p;
    *p = (unsigned short)(m >= max ? m-max : 0);
  }
}

Max in this example is truncated to max_short value, if it is greater than m, or just truncated to 16 bit, if it is not. It is vaid transformation, because if max > max_short, result of the expression will be zero.

Here is the table of types, I try to support, special case items are bold:

Size	128	256	512
i8	v16i8	v32i8	v64i8
i16	v8i16	v16i16	v32i16
i32		v8i32	v16i32
i64			v8i64

[zvi]

Please add some details about the combine in the Summary. It's better to put a description in addition to the links to make it easier to read the commit log.
Maybe something like:
Combine:
umax(a,b) - b -> subus(a,b)
a - umin(a,b) -> subus(a,b)

And better add an X86 label to the title.

lib/Target/X86/X86ISelLowering.cpp

35489

Would a better place for this comment be right after the '// Try to find ... ' comment?

35508

Please drop braces for if/else with a single statement

35538

Can we generalize by using DAG.computeKnownBits to tells us that the upper bits are zeroed?

[yulia_koval]

Added 512bit support and fixed comments

[zvi]

Please add the avx512vl feature to the AVX512 test (in the parent patch) and rebase this patch. It will drastically improve some of the cases :).

lib/Target/X86/X86ISelLowering.cpp

35645

Please drop the braces

[zvi]

test/CodeGen/X86/psubus.ll

2383 ↗

(On Diff #116797)

Looks like in this case SSE4.1, AVX1 and AVX2 regressed. Can you please take a look at it?

[yulia_koval]

Fixed problem on min case.

[zvi]

LGTM. Thanks!

[yulia_koval]

Thanks! Could you please commit it for me?

[spatel]

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

35928–35930 ↗

(On Diff #118259)

Sorry I didn't have a chance to look at this closely before commit, but why are we using isEqualTo() here? This pattern only applies to integer types, so we could use simple equality checks instead?

[yulia_koval]

Thanks, fixed your comment in additional patch below.

[spatel]

In D37534#895588, @yulia_koval wrote:

Thanks, fixed your comment in additional patch below.

Thank you - committed with rL315589.

lib/Target/X86/X86ISelLowering.cpp

     SubusRHS = Op1;
     SDValue MaxLHS = Op0.getOperand(0);
     SDValue MaxRHS = Op0.getOperand(1);
-    if (DAG.isEqualTo(MaxLHS, Op1))
+    if (MaxLHS == Op1)
       SubusLHS = MaxRHS;
-    else if (DAG.isEqualTo(MaxRHS, Op1))
+    else if (MaxRHS == Op1)
       SubusLHS = MaxLHS;
     else
       return SDValue();

     SubusLHS = Op0;
     SDValue MinLHS = Op1.getOperand(0);
     SDValue MinRHS = Op1.getOperand(1);
-    if (DAG.isEqualTo(MinLHS, Op0))
+    if (MinLHS == Op0)
       SubusRHS = MinRHS;
-    else if (DAG.isEqualTo(MinRHS, Op0))
+    else if (MinRHS == Op0)
       SubusRHS = MinLHS;
     else
       return SDValue();