[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.


================
Comment at: include/clang/Basic/FixedPoint.h:23
+
+class FixedPointNumber {
+ public:
----------------
rjmccall wrote:
> The established naming convention here — as seen in `APInt`, `APFloat`, `APValue`, etc. — would call this `APFixedPoint`.  Maybe that's not a great convention, but we should at least discuss deviating from it.
> 
> You might also want a type which encapsulates the details of a fixed-point type, i.e. the semantic width, scale, and saturating-ness.  (Like the "float semantics" of `APFloat`.)
> 
> I think a key question here is whether you want a FixedPointNumber to exactly represent the bit-pattern or just the semantic value.  I think it would eliminate a non-trivial source of bugs in this type if it just represents the semantic value, i.e. if a 16-bit unsigned fract value on a target where that uses a padded representation did not explicitly represent the padding bit, and then just added it back in in some accessor that asks for the bit-pattern.  Regardless, that should be clearly documented.
>  a 16-bit unsigned fract value on a target where that uses a padded representation did not explicitly represent the padding bit

So does that mean that the underlying APInt in this type would be 15 bits instead of 16, to avoid representing the padding? It feels a bit scary to throw around values with different internal representation than what other parts of the code (say, the target specification) have specified them to be.


================
Comment at: include/clang/Basic/FixedPoint.h:45
+
+  FixedPointNumber extend(unsigned Width) const {
+    llvm::APSInt ValCpy = Val_;
----------------
I'm not so sure that extension and truncation on their own are particularly meaningful operations on fixed-point values. I think you need to provide both a width and a scale for these kinds of operations so you can both resize and rescale the value simultaneously. Perhaps you can even provide a 'saturation width' that the value should be saturating on when converting.

You have the `convert` method, but it only takes a QualType, so if you need to convert to something that doesn't exist as a type, it's not enough. Maybe I'm overdesigning.


================
Comment at: include/clang/Basic/FixedPoint.h:57
+
+  llvm::APSInt getIntPart() const { return Val_ >> Scale_; }
+
----------------
As with the integer conversion in the earlier patch, this does not round toward zero. This might lead to surprising results.


================
Comment at: lib/AST/ASTContext.cpp:10303
+  if (Ty->isSaturatedFixedPointType()) {
+    FixedPointNumber MaxVal = Context.getFixedPointMax(Ty);
+    FixedPointNumber MinVal = Context.getFixedPointMin(Ty);
----------------
It is possible to perform saturation without extending and comparing, by looking at the bits above the 'imagined' sign bit in the resulting type. If they are all the same, then the value is in range, otherwise you must saturate.

Explicit comparison probably gets the point across better.


================
Comment at: lib/AST/ASTContext.cpp:10320
+
+  if (DstWidth > SrcWidth) Val_ = Val_.extend(DstWidth);
+
----------------
If you do rescaling before and after resizing, you can use `extOrTrunc` instead.


================
Comment at: lib/AST/ASTContext.cpp:10342
+
+  unsigned CommonWidth = std::max(Val_.getBitWidth(), OtherWidth);
+  ThisVal = ThisVal.extend(CommonWidth);
----------------
This width and scale commoning looks odd. If you have two types for which the width is the same but the scale is different, you will discard bits from the value with lower scale and the comparison might be wrong.

You need to find a width and scale that can fit all of the values of both.


Repository:
  rC Clang

https://reviews.llvm.org/D48661