[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.


================
Comment at: include/clang/AST/Type.h:6552
+// as a scaled integer.
+std::string FixedPointValueToString(unsigned Radix, unsigned Scale,
+                                    uint64_t Val);
----------------
This should probably take an APInt or APSInt.

Also, is there a reason to only have it produce unsigned numbers?


================
Comment at: include/clang/Basic/LangOptions.def:306
 LANGOPT(FixedPoint, 1, 0, "fixed point types")
+LANGOPT(SameFBits, 1, 0,
+        "unsigned and signed fixed point type having the same number of fractional bits")
----------------
Is it safe to have this as a flag? What about making it a target property?


================
Comment at: include/clang/Basic/TargetInfo.h:89
+  // corresponding unsaturated types.
+  unsigned char ShortAccumFBits, ShortAccumIBits;
+  unsigned char AccumFBits, AccumIBits;
----------------
I suspect it's still possible to calculate the ibits based on the fbits, even for _Accum.

Are the unsigned values needed? The fbits for unsigned _Fract are the same as for signed _Fract if SameFBits is set, and +1 otherwise. The same should go for unsigned _Accum, but I don't think it's entirely clear how this affects the integral part.


================
Comment at: include/clang/Lex/LiteralSupport.h:116
+  /// occurred when calculating the integral part of the scaled integer.
+  bool GetFixedPointValue(uint64_t &Val, unsigned Scale);
+
----------------
This should return an APInt. That way you won't run the risk of losing any bits due to overflow or extraneous precision.


================
Comment at: lib/AST/ExprConstant.cpp:9427
+      const APSInt &Value = Result.getInt();
+      if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow()) {
+        std::string Val =
----------------
This doesn't saturate properly. Is that coming in a later patch?


================
Comment at: lib/AST/ExprConstant.cpp:9429
+        std::string Val =
+            "-" + FixedPointValueToString(
+                      /*Radix=*/10, Info.Ctx.getTypeInfo(E->getType()).Width,
----------------
This would probably be simpler if the FixedPointValueToString function could produce signed values.


================
Comment at: lib/AST/StmtPrinter.cpp:1540
+      llvm_unreachable("Unexpected type for fixed point literal!");
+    case BuiltinType::ShortFract:
+      OS << "hr";
----------------
Format this more like the one above to make fewer lines.


================
Comment at: lib/AST/Type.cpp:3992
+
+std::string clang::FixedPointValueToString(unsigned Radix, unsigned Scale,
+                                           uint64_t Val) {
----------------
I think this would be better if it took a SmallString (or SmallVectorImpl) as reference and used that instead of returning a std::string.

Also should probably take an APInt/APSInt.


================
Comment at: lib/Lex/LiteralSupport.cpp:1045
 
+bool NumericLiteralParser::GetFixedPointValue(uint64_t &Val, unsigned Scale) {
+  assert(radix == 16 || radix == 10);
----------------
This should take an APInt (and use APInts for processing) to store the result in.

It should be possible to calculate exactly how many bits are needed to fit the literal before you start reading the digits. Overflow should not be a problem, but you might get precision loss in the fractional part. The calculation in our version is `ceil(log2(10^(noof-digits))) + Scale` but ours only handles normal decimal notation (123.456) so it might need to be extended to handle exponents and hex.


================
Comment at: lib/Sema/SemaExpr.cpp:1248
+  bool RHSFixed = RHSType->isFixedPointType();
+
+  if (LHSFixed && RHSFixed) {
----------------
I don't see how these semantics work properly. The specification requires that operations be done in the full precision of both types. You cannot convert the types before performing the operation like this, since the operation will not be done in full precision in that case.

The operator semantics of Embedded-C require the operand types of binary operators to be different. It's only when you've performed the operation that you are allowed to convert the result to the resulting type.


================
Comment at: lib/Sema/SemaExpr.cpp:1336
+  // Handle fixed point types
+  if (LHSType->isFixedPointType() || RHSType->isFixedPointType())
+    return handleFixedPointConversion(*this, LHS, RHS, LHSType, RHSType,
----------------
I guess you haven't gotten there yet, but this should probably be before handleFloatConversion if you want to handle 'float + _fract' later.


================
Comment at: lib/Sema/SemaExpr.cpp:3415
+    if (Literal.isFract) {
+      uint64_t MaxVal = 1ULL << scale;
+      if (Val > MaxVal) {
----------------
With APInts it should be possible to generalize this code to work with both accum and fract.


Repository:
  rC Clang

https://reviews.llvm.org/D46915