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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
