[PATCH] D55780: [X86] Create PSUBUS from (add (umax X, C), -C)

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Craig,

Your patch addresses the issue with the `subs` example from PR40053. However, as soon as we change the code to something like this, then your rule would not trigger:

  unsigned long long test_sub_2(__m128i x) {
      __m128i c = _mm_set1_epi8(70);
      return _mm_subs_epu8(x, c)[0];
  }

This is similar to the example from PR40053, with the difference that only element zero is effectively used.
If I build this with clang, the optimizer performs a `simplify demanded elts`-like optimization that propagates undefs to the constant vector.
That undef propagation later on breaks the UMAX pattern. So, it won't appear in the "Initial Selection DAG".
As a consequence, your new pattern would not work, and we end up with this codegen (avx):

  vpmaxub .LCPI1_0(%rip), %xmm0, %xmm1
  vpcmpeqb        %xmm1, %xmm0, %xmm1
  vpaddb  .LCPI1_1(%rip), %xmm0, %xmm0
  vpand   %xmm0, %xmm1, %xmm0
  vmovq   %xmm0, %rax

Your patch improves the matching of SUBUS, but more work would need to be done in this area.

There is also another problem caused by `undef` being aggressively propagated by the logic that simplifies demanded vector elements. See my comment below.



================
Comment at: lib/Target/X86/X86ISelLowering.cpp:40522
+  };
+  if (!ISD::matchBinaryPredicate(Op0.getOperand(1), Op1, MatchSUBUS))
+    return SDValue();
----------------
My understanding is that `matchBinaryPredicate()` bails out if not all elements from the input vectors are ConstantSDNode. In the presence of constant build vectors with some undefs, that logic would not be able to match a SUBUS. However, the presence of undef values shouldn't really matter in this particular case. It should be safe to ignore them.

Recently `simplify demanded vector elts` has become more effective (and a bit more aggressive). Being able to propagate `undef` to unused lanes of an input vector is great. However, in one particular case (D55600) it caused a regression.
The root cause of that regression was the the inability of `matchBinaryPredicate()` to handle undefs.

I am worried that, the more we improve that simplification logic, the more likely it is to end up propagating `undef` values to constant vectors.

I think that we need a version of `matchBinaryPredicate` that knows how to skip undefs. Alternatively, ``matchBinaryPredicate` could accept an extra (optional) bool argument named `IgnoreUndefs`.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55780/new/

https://reviews.llvm.org/D55780