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

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

You should not define the fixed-point precision as compiler macros at build configure time. The number of fractional bits (the scaling factor) should be added to TargetInfo as target-configurable variables/accessors, and an accessor should be added to ASTContext (we call it 'getFixedPointScale') to fetch the scaling factor of arbitrary fixed-point types.

As I mentioned on the mailing list, we would also like to solve the problem with different scaling factors on the signed and unsigned _Fract types. I'm not sure what the best approach is, but the simplest solution could perhaps be enabled with a target flag.

If the flag is true, the scaling factor of the unsigned _Fract types is the same as the signed ones and the MSB is padding. If the flag is false, the scaling factor of the unsigned _Fract types is one greater than the signed types and all of the bits are used.

-----

Overall, the literal parsing code seems very ad-hoc. Fixed-point values are just integers, it should be possible to process them in exact precision instead of relying on host system implementation details (using double) and floating point types.

I could provide patches from our downstream port, but there are a number of hurdles:

- DSP-C does a bunch of extra 'type promotion' when parsing fixed-point literals
- literals in DSP-C cannot contain the 'exponent notation' so the parsing routine cannot handle this
- the routine uses utility functions added to APInt in our LLVM branch

If you are interested I can share anyway so you can see how we have done it.



================
Comment at: include/clang/Lex/LiteralSupport.h:72
 
+  enum FixedPointType { FPT_UNSPECIFIED, FPT_ACCUM, FPT_FRACT };
+
----------------
Is there a reason this is not added as two fields 'isAccum' and 'isFract'?


================
Comment at: include/clang/Lex/LiteralSupport.h:75
+  // We use separate fields for fixed point sizes b/c the isHalf/isLong booleans
+  // assume that this literal is an integral type instead of fixed point type.
+  enum FixedPointSize { FPS_UNSPECIFIED, FPS_SHORT, FPS_LONG };
----------------
Shouldn't the flags be amended instead?


================
Comment at: lib/AST/ASTContext.cpp:1788
     case BuiltinType::UShortAccum:
+    case BuiltinType::ShortFract:
+    case BuiltinType::UShortFract:
----------------
See my comments on the _Accum patch.


================
Comment at: lib/AST/ASTDumper.cpp:2186
+  ColorScope Color(*this, ValueColor);
+  OS << " " << Node->getValue().toString(10, isSigned);
+}
----------------
This will not print as a fixed-point number.


================
Comment at: lib/AST/Expr.cpp:766
+  const auto *BT = type->getAs<BuiltinType>();
+  assert(BT && "Not a fixed point type!");
+  switch (BT->getKind()) {
----------------
Is this possible given the assertion above?


================
Comment at: lib/AST/Expr.cpp:767
+  assert(BT && "Not a fixed point type!");
+  switch (BT->getKind()) {
+    default:
----------------
There is no reason to have this switch.


================
Comment at: lib/AST/Expr.cpp:774
+    case BuiltinType::UShortFract:
+      assert(V.getBitWidth() == C.getIntWidth(type) &&
+             "Short fixed point type is not the correct size for constant.");
----------------
'getIntWidth' is likely the wrong accessor to use here. Fixed-point types are technically not ints.


================
Comment at: lib/AST/ExprConstant.cpp:9323
+      if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow() &&
+          !HandleOverflow(Info, E, -Value.extend(Value.getBitWidth() + 1),
+                          E->getType()))
----------------
Is signed fixed-point overflow actually considered undefined behavior? I'm not familiar with what Embedded-C says about this.

Also, this routine will likely print the wrong number for fixed-point values.


================
Comment at: lib/AST/ExprConstant.cpp:9328
+    }
+    case UO_Not: {
+      if (!Visit(E->getSubExpr())) return false;
----------------
I do not believe ~ is valid on fixed-point types.


================
Comment at: lib/AST/StmtPrinter.cpp:1532
+  bool isSigned = Node->getType()->isSignedFixedPointType();
+  OS << Node->getValue().toString(10, isSigned);
+
----------------
This will not print a fixed-point number.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:675
+  case CK_IntegralToFixedPoint:
+    llvm_unreachable(
+        "AggExprEmitter::VisitCastExpr CK_IntegralToFixedPoint");  // TODO
----------------
This probably goes in the large default case below, as fixed-point types are not aggregates.


================
Comment at: lib/CodeGen/CGExprComplex.cpp:451
+    llvm_unreachable(
+        "ComplexExprEmitter::EmitCast CK_IntegralToFixedPoint");  // TODO
   case CK_Dependent: llvm_unreachable("dependent cast kind in IR gen!");
----------------
Same as the aggregate case.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1789
+      case BuiltinType::ShortAccum:
+        fbits = BUILTIN_SACCUM_FBIT;
+        break;
----------------
This should all use ASTContext accessors.


================
Comment at: lib/Sema/SemaExpr.cpp:1334
 
+  // Handle floating point types
+  if (LHSType->isFixedPointType() || RHSType->isFixedPointType()) {
----------------
Fixed-point types.


================
Comment at: lib/Sema/SemaExpr.cpp:1335
+  // Handle floating point types
+  if (LHSType->isFixedPointType() || RHSType->isFixedPointType()) {
+    return handleFixedPointConversion(*this, LHS, RHS, LHSType, RHSType,
----------------
Braces are not needed.


================
Comment at: lib/Sema/SemaExpr.cpp:3385
+
+    QualType floatingTy = Context.DoubleTy;
+    const llvm::fltSemantics &Format =
----------------
You cannot parse the literal as a double and then convert it. You cannot control how the value is parsed and you might lose bits.

You should parse it explicitly by hand as a scaled integer large enough to carry the literal and then scale it down and truncate it when you know what the final type will be.


================
Comment at: lib/Sema/SemaExpr.cpp:3397
+      case FPS_UNSPECIFIED:
+        bit_width = Context.getTargetInfo().getIntWidth();
+        break;
----------------
These (bit_width, fbits etc) should be collected from ASTContext after the type has been determined.


================
Comment at: lib/Sema/SemaExpr.cpp:3407
+
+    if (Literal.fixedPointType == FPT_ACCUM) {
+      if (isSigned) {
----------------
It should be possible to do this in a much more concise manner. In our port we do this with a table lookup indexed by the type selection bits.


================
Comment at: lib/Sema/SemaExpr.cpp:3483
+      assert(
+          (fbits + ibits + 1 <= bit_width) &&
+          "The total fractional, integral, and sign bits exceed the bit width");
----------------
It does not feel like verifying this is the responsibility of the literal parser.


================
Comment at: lib/Sema/SemaExpr.cpp:3490
+
+    using llvm::APFloat;
+    APFloat Val(Format);
----------------
As I mentioned above, please do not use floating point routines to parse the fixed-point literals.



Repository:
  rC Clang

https://reviews.llvm.org/D46915