[PATCH] D57836: [Intrinsic] Unsigned Fixed Point Saturation Multiplication Intrinsic

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6724
       return DAG.getNode(ISD::MUL, dl, VT, LHS, RHS);
-    } else if (Saturating && isOperationLegalOrCustom(ISD::SMULO, VT)) {
+    } else if (Signed && isOperationLegalOrCustom(ISD::SMULO, VT)) {
       SDValue Result =
----------------
RKSimon wrote:
> Can't we now get here if !Saturating &&  !isOperationLegalOrCustom(ISD::MUL, VT)  ?
Yes, that would be wrong! I will restore this to fall through to check for SMUL_LOHI, MULHS etc.

Another idea is to disallow scale==0 for SMULFIX and UMULFIX. SelectionDAGBuilder could just select an ordinary MUL instead for smul.fix when scale is zero.
Similarly umul.fix/umul.fix.sat with scale==VTSize could be selected as MULHU instead of UMULFIX and UMULFIXSAT (getting rid of some more special cases).




================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6810
+  APInt MaxVal = APInt::getSignedMaxValue(VTSize);
+  SDValue LowMask = DAG.getConstant(APInt::getLowBitsSet(VTSize, Scale - 1),
+                                    dl, VT);
----------------
This does not work for Scale==0 (and we might pass through the special handling for Scale==0 above. The same problem exists in the old code.
I'll need to look into this a bit more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57836