[Fixed Point Arithmetic] Fix for FixedPointValueToString

[leonardchan]

Summary

• Print negative numbers correctly
• Handle APInts of different sizes
• Add formal unit tests for FixedPointValueToString
• Add tests for checking correct printing when padding is set
• Restrict to printing in radix 10 since that's all we need for now

[ebevhan]

LGTM.

[ebevhan]

lib/AST/Expr.cpp

788

Unrelated to this patch specifically, but using getZExtValue here is a bit limiting.

[leonardchan]

lib/AST/Expr.cpp

788

I'll make another small patch to change this since should also do signed values now,

include/clang/AST/Type.h

 QualType DecayedType::getPointeeType() const {
   QualType Decayed = getDecayedType();
   (void)AttributedType::stripOuterNullability(Decayed);
   return cast<PointerType>(Decayed)->getPointeeType();
 }
 
 // Get the decimal string representation of a fixed point type, represented
 // as a scaled integer.
-void FixedPointValueToString(SmallVectorImpl<char> &Str,
-                             const llvm::APSInt &Val,
-                             unsigned Scale, unsigned Radix);
+void FixedPointValueToString(SmallVectorImpl<char> &Str, llvm::APSInt Val,
+                             unsigned Scale);
 
 } // namespace clang
 
 #endif // LLVM_CLANG_AST_TYPE_H

lib/AST/Expr.cpp

 }
 
 std::string FixedPointLiteral::getValueAsString(unsigned Radix) const {
   // Currently the longest decimal number that can be printed is the max for an
   // unsigned long _Accum: 4294967295.99999999976716935634613037109375
   // which is 43 characters.
   SmallString<64> S;
   FixedPointValueToString(
-      S, llvm::APSInt::getUnsigned(getValue().getZExtValue()), Scale, Radix);
+      S, llvm::APSInt::getUnsigned(getValue().getZExtValue()), Scale);
   return S.str();
 }
 
 FloatingLiteral::FloatingLiteral(const ASTContext &C, const llvm::APFloat &V,
                                  bool isexact, QualType Type, SourceLocation L)
   : Expr(FloatingLiteralClass, Type, VK_RValue, OK_Ordinary, false, false,
          false, false), Loc(L) {
   setSemantics(V.getSemantics());

lib/AST/ExprConstant.cpp

case UO_Plus:
       return Visit(E->getSubExpr());
     case UO_Minus: {
       if (!Visit(E->getSubExpr())) return false;
       if (!Result.isInt()) return Error(E);
       const APSInt &Value = Result.getInt();
       if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow()) {
         SmallString<64> S;
         FixedPointValueToString(S, Value,
-                                Info.Ctx.getTypeInfo(E->getType()).Width,
-                                /*Radix=*/10);
+                                Info.Ctx.getTypeInfo(E->getType()).Width);
         Info.CCEDiag(E, diag::note_constexpr_overflow) << S << E->getType();
         if (Info.noteUndefinedBehavior()) return false;
       }
       return Success(-Value, E);
     }
     case UO_LNot: {
       bool bres;
       if (!EvaluateAsBooleanCondition(E->getSubExpr(), bres, Info))

lib/AST/Type.cpp

QualType::DestructionKind QualType::isDestructedTypeImpl(QualType type) {
   return DK_none;
 }
 
 CXXRecordDecl *MemberPointerType::getMostRecentCXXRecordDecl() const {
   return getClass()->getAsCXXRecordDecl()->getMostRecentNonInjectedDecl();
 }
 
 void clang::FixedPointValueToString(SmallVectorImpl<char> &Str,
-                                    const llvm::APSInt &Val, unsigned Scale,
-                                    unsigned Radix) {
-  llvm::APSInt ScaleVal = llvm::APSInt::getUnsigned(1ULL << Scale);
-  llvm::APSInt IntPart = Val / ScaleVal;
-  llvm::APSInt FractPart = Val % ScaleVal;
-  llvm::APSInt RadixInt = llvm::APSInt::getUnsigned(Radix);
+                                    llvm::APSInt Val, unsigned Scale) {
+  if (Val.isSigned() && Val.isNegative() && Val != -Val) {
+    Val = -Val;
+    Str.push_back('-');
+  }
+
+  llvm::APSInt IntPart = Val >> Scale;
+
+  // Add 4 digits to hold the value after multiplying 10 (the radix)
+  unsigned Width = Val.getBitWidth() + 4;
+  llvm::APInt FractPart = Val.zextOrTrunc(Scale).zext(Width);
+  llvm::APInt FractPartMask = llvm::APInt::getAllOnesValue(Scale).zext(Width);
+  llvm::APInt RadixInt = llvm::APInt(Width, 10);
 
-  IntPart.toString(Str, Radix);
+  IntPart.toString(Str, /*radix=*/10);
   Str.push_back('.');
   do {
-    (FractPart * RadixInt / ScaleVal).toString(Str, Radix);
-    FractPart = (FractPart * RadixInt) % ScaleVal;
-  } while (FractPart.getExtValue());
+    (FractPart * RadixInt)
+        .lshr(Scale)
+        .toString(Str, /*radix=*/10, Val.isSigned());
+    FractPart = (FractPart * RadixInt) & FractPartMask;
+  } while (FractPart != 0);
 }

test/Frontend/fixed_point_to_string.c

+// RUN: %clang_cc1 -ast-dump -ffixed-point %s | FileCheck %s
+// RUN: %clang_cc1 -ast-dump -ffixed-point -fpadding-on-unsigned-fixed-point %s | FileCheck %s
+
+/**
+ * Check the same values are printed in the AST regardless of if unsigned types
+ * have the same number of fractional bits as signed types.
+ */
+
+unsigned short _Accum u_short_accum = 0.5uhk;
+unsigned _Accum u_accum = 0.5uk;
+unsigned long _Accum u_long_accum = 0.5ulk;
+unsigned short _Fract u_short_fract = 0.5uhr;
+unsigned _Fract u_fract = 0.5ur;
+unsigned long _Fract u_long_fract = 0.5ulr;
+
+//CHECK: FixedPointLiteral {{.*}} 'unsigned short _Accum' 0.5
+//CHECK: FixedPointLiteral {{.*}} 'unsigned _Accum' 0.5
+//CHECK: FixedPointLiteral {{.*}} 'unsigned long _Accum' 0.5
+//CHECK: FixedPointLiteral {{.*}} 'unsigned short _Fract' 0.5
+//CHECK: FixedPointLiteral {{.*}} 'unsigned _Fract' 0.5
+//CHECK: FixedPointLiteral {{.*}} 'unsigned long _Fract' 0.5

unittests/Frontend/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(FrontendTests
   ASTUnitTest.cpp
   CompilerInstanceTest.cpp
+  FixedPointString.cpp
   FrontendActionTest.cpp
   CodeGenActionTest.cpp
   ParsedSourceLocationTest.cpp
   PCHPreambleTest.cpp
   OutputStreamTest.cpp
   )
 target_link_libraries(FrontendTests
   PRIVATE
   clangAST
   clangBasic
   clangFrontend
   clangLex
   clangSema
   clangCodeGen
   clangFrontendTool
   )

unittests/Frontend/FixedPointString.cpp

+#include "clang/AST/Type.h"
+#include "llvm/ADT/APSInt.h"
+#include "llvm/ADT/SmallString.h"
+#include "gtest/gtest.h"
+
+using clang::FixedPointValueToString;
+using llvm::APSInt;
+using llvm::SmallString;
+
+namespace {
+
+TEST(FixedPointString, DifferentTypes) {
+  SmallString<64> S;
+  FixedPointValueToString(S, APSInt::get(320), 7);
+  ASSERT_STREQ(S.c_str(), "2.5");
+
+  S.clear();
+  FixedPointValueToString(S, APSInt::get(0), 7);
+  ASSERT_STREQ(S.c_str(), "0.0");
+
+  // signed short _Accum
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMaxValue(16, /*Unsigned=*/false), 7);
+  ASSERT_STREQ(S.c_str(), "255.9921875");
+
+  // signed _Accum
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMaxValue(32, /*Unsigned=*/false), 15);
+  ASSERT_STREQ(S.c_str(), "65535.999969482421875");
+
+  // signed long _Accum
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMaxValue(64, /*Unsigned=*/false), 31);
+  ASSERT_STREQ(S.c_str(), "4294967295.9999999995343387126922607421875");
+
+  // unsigned short _Accum
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMaxValue(16, /*Unsigned=*/true), 8);
+  ASSERT_STREQ(S.c_str(), "255.99609375");
+
+  // unsigned _Accum
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMaxValue(32, /*Unsigned=*/true), 16);
+  ASSERT_STREQ(S.c_str(), "65535.9999847412109375");
+
+  // unsigned long _Accum
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMaxValue(64, /*Unsigned=*/true), 32);
+  ASSERT_STREQ(S.c_str(), "4294967295.99999999976716935634613037109375");
+
+  // signed short _Fract
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMaxValue(8, /*Unsigned=*/false), 7);
+  ASSERT_STREQ(S.c_str(), "0.9921875");
+
+  // signed _Fract
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMaxValue(16, /*Unsigned=*/false), 15);
+  ASSERT_STREQ(S.c_str(), "0.999969482421875");
+
+  // signed long _Fract
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMaxValue(32, /*Unsigned=*/false), 31);
+  ASSERT_STREQ(S.c_str(), "0.9999999995343387126922607421875");
+
+  // unsigned short _Fract
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMaxValue(8, /*Unsigned=*/true), 8);
+  ASSERT_STREQ(S.c_str(), "0.99609375");
+
+  // unsigned _Fract
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMaxValue(16, /*Unsigned=*/true), 16);
+  ASSERT_STREQ(S.c_str(), "0.9999847412109375");
+
+  // unsigned long _Fract
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMaxValue(32, /*Unsigned=*/true), 32);
+  ASSERT_STREQ(S.c_str(), "0.99999999976716935634613037109375");
+}
+
+TEST(FixedPointString, Negative) {
+  SmallString<64> S;
+  FixedPointValueToString(S, APSInt::get(-320), 7);
+  ASSERT_STREQ(S.c_str(), "-2.5");
+
+  S.clear();
+  FixedPointValueToString(S, APSInt::get(-64), 7);
+  ASSERT_STREQ(S.c_str(), "-0.5");
+
+  // signed short _Accum
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMinValue(16, /*Unsigned=*/false), 7);
+  ASSERT_STREQ(S.c_str(), "-256.0");
+
+  // signed _Accum
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMinValue(32, /*Unsigned=*/false), 15);
+  ASSERT_STREQ(S.c_str(), "-65536.0");
+
+  // signed long _Accum
+  S.clear();
+  FixedPointValueToString(S, APSInt::getMinValue(64, /*Unsigned=*/false), 31);
+  ASSERT_STREQ(S.c_str(), "-4294967296.0");
+}
+
+} // namespace