[PATCH] D46927: [Fixed Point Arithmetic] Augmented Assignment for Fixed Point Types

Bevin Hansson via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed May 23 02:30:56 PDT 2018

ebevhan added inline comments.

Comment at: include/clang/AST/Type.h:6577
+inline unsigned getFixedPointFBits(const QualType &Ty) {
+  return getFixedPointFBits(*Ty);
As I mentioned in another patch, this should be in ASTContext and ask TargetInfo for the information.

I'm not a fan of the term 'fbits'. The correct terminology should be the 'scaling factor'.

Comment at: lib/CodeGen/CGExprScalar.cpp:1009
+  // a float and perform some floating point arithmetic, though this may cost
+  // more. Enable that if #pragma FX_FULL_PRECISION is provided.
+  if (dest_fbits > src_fbits) {
For future reference, please do not rely on the behavior of floating point for fixed-point operations.

Comment at: lib/CodeGen/CGExprScalar.cpp:1184
+    // Casting down, so we will need to shift early as to not lose data
+    Src = EmitFixedPointRadixShift(Src, SrcType, DstType);
+  }
I think it's safer to simply do all relevant fixed-point conversion by hand here instead of falling through and relying on the semantics of other conversions.

We can provide patches for our implementation here if wanted.

Comment at: lib/CodeGen/CGExprScalar.cpp:1214
+  if (WorkingOnFixedPoints && order >= 0) {
+    // Casting up (or same type), so we can safely shift without losing data
This case should not be needed. Perform all fixed-point conversion before the integer type check above and do an early return instead.

It's not safe to rely on the behavior of other conversions for the behavior of these.

  rC Clang


More information about the cfe-commits mailing list