leonardchan 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);
----------------
ebevhan wrote:
> This should probably take an APInt or APSInt.
>
> Also, is there a reason to only have it produce unsigned numbers?
Done. It initially took only unsigned numbers since the literals could only be
unsigned, but it makes sense to take signed also.
================
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")
----------------
ebevhan wrote:
> Is it safe to have this as a flag? What about making it a target property?
Can do, but if the point of this option is just to set the Unsigned FBits to
their corresponding Signed FBits, then would it be better instead to just
ignore this flag altogether and have the target explicitly override the default
Unsigned values with the corresponding Signed vals?
================
Comment at: include/clang/Basic/TargetInfo.h:89
+ // corresponding unsaturated types.
+ unsigned char ShortAccumFBits, ShortAccumIBits;
+ unsigned char AccumFBits, AccumIBits;
----------------
ebevhan wrote:
> 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.
Similar to the previous comment, if we choose to make SameFBits dependent
on/controlled by the target, then I think it would be better for the target to
explicitly specify the integral bits since there could be some cases where
there may be padding (such as for unsigned _Accum types if this flag is set),
and in this case, I think it should be explicit that the `bit_width != IBits +
FBits`.
We also can't fill in that padding for the unsigned _Accum types as an extra
integral bit since the standard says that `"each signed accum type has at least
as many integral bits as its corresponding unsigned accum type".`
For the unsigned _Fract values, I think we can get rid of them if we choose to
keep the flag instead, but it would no longer apply to unsigned _Accum types
since it would allow for extra padding in these types and would conflict with
the logic of `bit_width == IBits + FBits`
For now, I actually think the simplest option is to keep these target
properties, but have the target override them individually, with checks to make
sure the values adhere to the standard.
================
Comment at: lib/AST/ExprConstant.cpp:9427
+ const APSInt &Value = Result.getInt();
+ if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow()) {
+ std::string Val =
----------------
ebevhan wrote:
> This doesn't saturate properly. Is that coming in a later patch?
Handling of saturated types will come in a later patch. For now this is focused
on literals for which we cannot specify saturation.
================
Comment at: lib/Lex/LiteralSupport.cpp:1045
+bool NumericLiteralParser::GetFixedPointValue(uint64_t &Val, unsigned Scale) {
+ assert(radix == 16 || radix == 10);
----------------
ebevhan wrote:
> 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.
I see. For the decimal exponent, the number of extra bits needed I think is
`ceil(Exponent * log2(10))` which requires parsing the exponent before parsing
the integral or fractional part, assuming the exponent is positive.
For hex, the number of digits needed just for the integral and fractional part
is just 4x the number of digits given in the hex literal + the exponent if it
is positive.
With a bit more math we could also trim down the number of digits needed, but I
think it's safe just to use the max number of digits needed.
================
Comment at: lib/Sema/SemaExpr.cpp:1248
+ bool RHSFixed = RHSType->isFixedPointType();
+
+ if (LHSFixed && RHSFixed) {
----------------
ebevhan wrote:
> 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.
Initially the idea was to convert both sides to fixed point types, then perform
standard binary operations between the fixed point types.
For the example, a `fract * int` would have the int converted to a fixed point
type by left shifting it by the scale of the fract, multiplying, then right
shifting by the scale again to get the resulting fract. The only unhandled
thing is overflow, but the precision of the fract remains the same. The
operands would also be casted up beforehand so there was enough space to store
the result, which was casted down back to the original fract after performing
the right shift by the scale.
Operations between fixed point types would follow a similar process of casting
both operands to the higher rank fixed point type, and depending on the
operation, more underlying shifting and casting would be done to retain full
precision of the higher ranked type.
Though I will admit that I did not realize until now that multiplying a fixed
point type by an integer does not require shifting the integer.
================
Comment at: lib/Sema/SemaExpr.cpp:1336
+ // Handle fixed point types
+ if (LHSType->isFixedPointType() || RHSType->isFixedPointType())
+ return handleFixedPointConversion(*this, LHS, RHS, LHSType, RHSType,
----------------
ebevhan wrote:
> I guess you haven't gotten there yet, but this should probably be before
> handleFloatConversion if you want to handle 'float + _fract' later.
A later patch adds a case in `handleFloatConversion` where we check if one of
the operands is an integer and performs a cast from there.
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
