[PATCH] D54719: [Intrinsic] Signed Fixed Point Multiplication Intrinsic

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

Need to update LangRef.rst which I think we also missed for the saturating intrinsics.



================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:278
+    /// both operands as fixed point numbers. A scale of zero is effectively
+    /// performing saturation multiplication on 2 integers.
+    SMULFIX,
----------------
This is says it saturates but I didn't see that implemented in the expansion function.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:625
+  SDValue Op2 = N->getOperand(1);
+  SDValue Op1Promoted = GetPromotedInteger(Op1);
+  SDValue Op2Promoted = GetPromotedInteger(Op2);
----------------
Get rid of Op1 and Op2 and just do GetPromotedInteger(N->getOperand(0))


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5093
+    SDValue Hi = DAG.getNode(ISD::MULHS, dl, VT, LHS, RHS);
+    Lo = DAG.getNode(ISD::SRL, dl, VT, Lo, DAG.getConstant(Scale, dl, VT));
+    Hi = DAG.getNode(ISD::SHL, dl, VT, Hi,
----------------
Shift amount constants should get their type from getShiftAmountTy.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5096
+                     DAG.getConstant(VT.getScalarSizeInBits() - Scale, dl, VT));
+    return DAG.getNode(ISD::ADD, dl, VT, Lo, Hi);
+  } else {
----------------
This is really an OR isn't it? DAG combiner will turn it into that so might as well just use OR.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5097
+    return DAG.getNode(ISD::ADD, dl, VT, Lo, Hi);
+  } else {
+    return Lo;
----------------
No need for an else after a return.


================
Comment at: llvm/lib/IR/Verifier.cpp:4531
+    Assert(Op3->getType()->isIntegerTy(),
+           "third argumenr of smul_fix must be an int");
+    break;
----------------
argumenr->argument


================
Comment at: llvm/lib/IR/Verifier.cpp:4532
+           "third argumenr of smul_fix must be an int");
+    break;
+  }
----------------
I think you need to check that Op3 is a ConstantInt as well. And that it fits in 32 bits.


Repository:
  rL LLVM

https://reviews.llvm.org/D54719