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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:40522
+  };
+  if (!ISD::matchBinaryPredicate(Op0.getOperand(1), Op1, MatchSUBUS))
+    return SDValue();
----------------
andreadb wrote:
> 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`.
> 
I'm looking at this now - D55819 does the same for matchUnaryPredicate and I should have the equivalent matchBinaryPredicate patch available soon


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

https://reviews.llvm.org/D55780