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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5420
-          Node->getOpcode() == ISD::UMULFIX) &&
-         "Expected opcode to be SMULFIX or UMULFIX.");
-
----------------
Why drop the assert? Why not just add ISD::SMULFIXSAT tests?


================
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)) {
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55720