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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits