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

[Bevin Hansson via Phabricator via cfe-commits]

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.


Repository:
  rC Clang

https://reviews.llvm.org/D46927