[PATCH] D55625: [Intrinsic] Unsigned Fixed Point Multiplication Intrinsic

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.

In D55625#1331183 <https://reviews.llvm.org/D55625#1331183>, @ebevhan wrote:

> Maybe a test with scale == bitwidth? Other than that it looks good to me.


Added



================
Comment at: llvm/docs/LangRef.rst:13010
+
+It is undefined behavior if the source value does not fit within the range of
+the fixed point type.
----------------
bjope wrote:
> As a reader of this document (or rather a user of the intrinsic) I'd assume that the fixed point type is given by the width of the source arguments, together with the scale. So how can a source value not be within range?
This should actually say `result value`, not the source value.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5843
   }
+  case Intrinsic::umul_fix: {
+    SDValue Op1 = getValue(I.getArgOperand(0));
----------------
ebevhan wrote:
> Is there a reason this doesn't reuse the code above?
Initially this was because when choosing the opcode, I would have another nested switch stmt, but since I'll also have very similar code for the saturating versions of SMULFIX and UMULFIX, having this switch could save some lines.

I imagine this same question could be applied to the add/sub sat intrinsics and have them squished together.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55625