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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/docs/LangRef.rst:12604
+      declare i64 @llvm.smul.fix.i64(i64 %a, i64 %b, i32 %scale)
+      declare <4 x i32> @llvm.smul.fix.v4i32(<4 x i32> %a, <4 x i32> %b, i32 %scale)
+
----------------
I'm not sure if/how the auto-vectorizers are aware that only the first two operands should be vectorized and that the third operand should stay scalar. So I guess it is unlikely that we ever get any vector operands at the moment (except for handwritten lit tests).

So I guess this should work for now (after all it is simpler than having a vector of scales). But perhaps we need to allow the scale to be given as a vector as well in the future to support vectorization (`shl` is for example similar, but afaik it will get the shift counts as a vector when being vectorized).



================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:801
+  /// Custom method defined by each target to indicate if an operation which
+  /// may require a saturation bit width is supported natively by the target.
+  /// If not, the operation is illegal.
----------------
Shouldn't this say "scale" instead of "saturation bit width"?


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:819
+    // This operation is supported in this type but may only work on specific
+    // saturation widths.
+    bool Supported;
----------------
Same as above, isn't the check about "scales" and not "saturation widths"?


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:742
 
 //===------------------------- Fixed Point Intrinsics ---------------------===//
 //
----------------
nit: In the langref you are using "Saturation Arithmetic Intrinsics" and "Fixed Point Arithemetic Instrinsics" as two separate chapters. And here we put all of them into one category "Fixed Point Intrinsics". I'm not sure if the saturating arithmetics should be in a fixed point category.

Anyway, when adding a int_smul_fix_sat later I guess it will be in the "Fixed Point Arithemetic Instrinsics" in the langref (even if it also is saturating). So maybe it is better to not having this split into two categories after all.


Repository:
  rL LLVM

https://reviews.llvm.org/D54719