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

[Bevin Hansson via Phabricator via llvm-commits]

ebevhan added inline comments.


================
Comment at: llvm/docs/LangRef.rst:12628
+this scale, the value is either rounded down to the closest fixed point value
+less than the source value, or rounded ip to the closest fixed point value
+greater than the source value. This rounding is dependent on the target.
----------------
typo "up"


================
Comment at: llvm/docs/LangRef.rst:12629
+less than the source value, or rounded ip to the closest fixed point value
+greater than the source value. This rounding is dependent on the target.
+
----------------
This doc doesn't say what happens on overflow. Truncation/wraparound? Or should we consider it undefined?

If it says that the rounding is dependent on the target, will the rounding mode be target-configurable and exposed through TLI/TTI? We essentially can't touch these intrinsics (constant folding, optimization, even legalization) otherwise.

Looking at the legalization sequence, it will definitely lower these into a round-down form. If a target has some legal operations which round up and some non-legal operations, then the legal ones will round up and the non-legal ones will round down. That's sort of messy.

It might be safer to say that the rounding is indeterminate, but that's even worse for optimization.


================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:278
+    /// both operands as fixed point numbers. A scale of zero is effectively
+    /// performing multiplication on 2 integers.
+    SMULFIX,
----------------
The intrinsic docs in the LangRef mentions that the last value must be constant, but this comment doesn't.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:3785
+  /// Method for building the DAG expansion of ISD::SMULFIX. This method accepts
+  /// integers or vectors of integers as its arguments.
+  SDValue getExpandedFixedPointMultiplication(SDNode *Node,
----------------
This comment clashes with the assertions in the function. It doesn't take vectors.


================
Comment at: llvm/include/llvm/Target/TargetSelectionDAG.td:383
 def usubsat    : SDNode<"ISD::USUBSAT"   , SDTIntBinOp>;
+def smulfix    : SDNode<"ISD::SMULFIX"   , SDTIntBinOp, [SDNPCommutative]>;
 
----------------
It's marked SDTIntBinOp, but is supposed to have three input operands. I think these nodes might need a new SDT.

Also, multiplication is obviously commutative, but I don't know if SDNPCommutative works on nodes that have anything except two operands. It might not have an effect at all. Someone who knows more about DAG might have more info on that.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2543
+          DAG.getNode(ISD::MUL, dl, VT, LHS, RHS).getNode(), Lo, Hi, NVT, DAG,
+          TargetLowering::MulExpansionKind::OnlyLegalOrCustom, LL, LH, RL, RH))
+    return;
----------------
Maybe I'm missing something here, but isn't this just a normal, expanded multiplication?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2544
+          TargetLowering::MulExpansionKind::OnlyLegalOrCustom, LL, LH, RL, RH))
+    return;
+
----------------
If we hit this return, doesn't this mean that legalization failed? Do we want to catch this here?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5101
+  if (Scale) {
+    SDValue Hi = DAG.getNode(ISD::MULHS, dl, VT, LHS, RHS);
+    EVT ShiftTy = getShiftAmountTy(VT, DAG.getDataLayout());
----------------
Could use a couple comments explaining what we're doing with the values/SRL/SHL.

Does this work if MULHS in VT is of dubious legality?


Repository:
  rL LLVM

https://reviews.llvm.org/D54719