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

[Leonard Chan via Phabricator via cfe-commits]

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