[PATCH] D46986: [Fixed Point Arithmetic] Validation Test for Fixed Point Binary Operations and Saturated Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I cannot say that I'm pleased with the CodeGen emission of the operations as pure IR. I can only assume that you do not have hardware specifically tailored for these operations, as matching this type of code ought to be quite difficult after optimization is performed.



================
Comment at: include/clang/AST/Type.h:6591
+// saturated, return back the type itself.
+QualType getCorrespondingSaturatedFixedPointType(ASTContext &Context,
+                                                 const Type &Ty);
----------------
If these need to be passed a ASTContext anyway, why not have these functions on ASTContext to begin with?


================
Comment at: include/clang/Basic/FixedPoint.h.in:35
+// Max values of each _Accum type as integer bytes
+#define SACCUM_MAX_AS_INT       ((1ULL << (BUILTIN_SACCUM_FBIT + BUILTIN_SACCUM_IBIT)) - 1)
+#define ACCUM_MAX_AS_INT        ((1ULL << (BUILTIN_ACCUM_FBIT + BUILTIN_ACCUM_IBIT)) - 1)
----------------
As mentioned in other patches, these should not be macros (this entire file should probably be removed altogether).


================
Comment at: lib/CodeGen/CGExprScalar.cpp:3151
+    const auto &BT = op.Ty->getAs<BuiltinType>();
+    switch (BT->getKind()) {
+      default:
----------------
All of these values can clearly be calculated based on the scaling factor and the width of the type.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:3206
+    if (op.Ty->isSignedFixedPointType()) {
+      MSBBitShift = getFixedPointIBits(op.Ty) + getFixedPointFBits(op.Ty);
+    } else {
----------------
Factor out conversion between fixed-point types into its own function so it can be reused for other cases, such as for other operations and actual conversions. It should probably not take QualTypes to convert to but rather arbitrary widths and scales, so it can be used to upscale to/downscale from 'intermediate' common calculation types.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:3211
+
+    llvm::Value *Sum = Builder.CreateAdd(op.LHS, op.RHS);
+    llvm::Value *LHSMSB = Builder.CreateLShr(op.LHS, MSBBitShift);
----------------
I would much rather see these operations emitted as intrinsics rather than straight IR... but I know that wasn't part of your proposal.


================
Comment at: lib/Sema/SemaExpr.cpp:1264
+/// integers or other fixed point types due to potential loss of precision.
+/// For this case of fixed point types, the resulting type in a binary operation
+/// does not need to be exactly one of the 2 operand types.
----------------
This is incorrect. The resulting type of a binary operation is absolutely one of the two operands. However, the calculation might be done as a type that is not one of the two, as it must be done in the full (combined) precision of both operands.

That is a detail reserved for CodeGen, however.


================
Comment at: lib/Sema/SemaExpr.cpp:3551
     uint64_t int_part_as_int = static_cast<uint64_t>(int_part);
+    uint64_t fract_part_as_int =
+        static_cast<uint64_t>(fract_part * (1ULL << fbits));
----------------
Should these changes not be part of the patch that adds the literal parsing code?


Repository:
  rC Clang

https://reviews.llvm.org/D46986