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