[PATCH] D55720: [Intrinsic] Signed Fixed Point Saturation Multiplication Intrinsic

[Leonard Chan via Phabricator via llvm-commits]

leonardchan marked 2 inline comments as done.
leonardchan added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5428
   if (!Scale) {
-    if (VT.isVector() && !isOperationLegalOrCustom(ISD::MUL, VT))
-      return SDValue();
-    return DAG.getNode(ISD::MUL, dl, VT, LHS, RHS);
+    // [us]mul.fix(a, b, 0) -> mul(a, b)
+    if (!Saturating && isOperationLegalOrCustom(ISD::MUL, VT)) {
----------------
RKSimon wrote:
> leonardchan wrote:
> > RKSimon wrote:
> > > I think you need something like this here (please double check my logic):
> > > ```
> > > if (VT.isVector() && !isOperationLegalOrCustom(ISD::SMULO, VT) &&
> > >     !(!Saturating && isOperationLegalOrCustom(ISD::MUL, VT)))
> > >   return SDValue(); // unroll
> > > ```
> > > And that will let you avoid the return SDValue() below by always defaulting to a scalar ISD::MUL/ISD::SMULO that legalization can handle.
> > I thought we still want to allow vectors to pass to calls to `MUL` and `SMULO`? Wouldn't this scalarize when we disallow vectors even if `MUL` and `SMULO` are legeal?
> I've committed rL353546 which /should/ mean that the scale==0 case is now safe to drop through.
Dropped and can confirm this works for my tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D55720