[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I think I've already expressed that I'm not at all a fan of encoding the full-precision logic in the operations themselves and not explicitly in the AST. We already have (well, not yet, but we will) routines for emitting conversions to/from/between fixed-point types of arbitrary semantics - why not use that instead of reimplementing the same logic for every binary operation?

As it is now, EmitFixedPointAdd replicates the logic for both converting from integer to fixed-point and fixed-point to fixed-point. You mention a special case with saturating addition, but this special case only exists because the routine needs to do fixed-point->saturating fixed-point on its own. If all EmitFixedPointAdd did was perform an add between two fixed-point values with the same semantics, then the logic would be much simpler and the act of conversion would be delegated to the routines that actually handle it.

I want to float the idea again of adding an AST type to encapsulate an arbitrary fixed-point semantic and using that as the 'common type' for binary operations involving fixed-point types. This would enable UsualArithmeticConversions to handle fixed-point types similarly to how it does today (find the 'common' full precision type, implicitly cast the LHS and RHS if necessary). There is one new thing that it would have to do; the result after the operation should not be the full precision type, but rather be casted to the operand type of highest rank. I don't think the existing code in SemaExpr can handle the case of adding an implicit cast after the operation. I don't think it should be hard to add, though.



================
Comment at: clang/lib/Sema/SemaExpr.cpp:1393
+/// rules in N1169 4.1.4.
+QualType Sema::FixedPointConversions(ExprResult &FixedExpr,
+                                     ExprResult &OtherExpr, bool IsCompAssign) {
----------------
I'm not sure I understand why this is in a separate function. What part of this doesn't work in UsualArithmeticConversions, in a 'handleFixedPointConversion' similar to the other cases?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1416
+  // converted to its corresponding signed fixed-point type and the resulting
+  // type is the type of the converted operand.
+  if (OtherTy->isSignedFixedPointType() &&
----------------
I feel like this logic can be folded into the rank handling. Does it work properly if you give signed types higher rank than unsigned ones?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1435
+  // account rounding and overflow) to the precision of the resulting type.
+  if (OtherTy->isIntegerType())
+    return FixedTy;
----------------
This can be avoided by making all integer types lower rank than the fixed point types. The rank between them doesn't matter, either; they can all be the same.


Repository:
  rC Clang

https://reviews.llvm.org/D53738