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