[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();
----------------
RKSimon wrote:
> 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
D55822 is for matchBinaryPredicate undef handling - I have no objections for this patch to go in first and I can add support for it to D55822


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

https://reviews.llvm.org/D55780