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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

You might need PromoteIntOp_SMULFIX for certain targets, like 64-bit RISCV.



================
Comment at: llvm/docs/LangRef.rst:12830
+value is rounded up or down to the closest representable value. The rounding
+direction is unspecified.
+
----------------
"The rounding direction is unspecified" seems scary to me... is there really no standard for rounding these operations?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:624
+  SDValue Op1Promoted = GetPromotedInteger(N->getOperand(0));
+  SDValue Op2Promoted = GetPromotedInteger(N->getOperand(1));
+  EVT PromotedType = Op1Promoted.getValueType();
----------------
Do you need to sign-extend here?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:3290
+SDValue DAGTypeLegalizer::ExpandIntOp_ADDCARRY(SDNode *N) {
+  SDValue Lo, Hi;
+  GetExpandedInteger(N->getOperand(2), Lo, Hi);
----------------
Maybe worth adding a comment that the operand that needs to be expanded must be the third operand, since the other two operands have the same type as the result.

And actually, I'm not sure how you would hit this in practice; it seems unusual to split a boolean value.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54719