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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: include/clang/Basic/FixedPoint.h:23
+
+class FixedPointNumber {
+ public:
----------------
rjmccall wrote:
> ebevhan wrote:
> > 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.
> Well, you can encapsulate it so that the 15-bit APInt is never exposed to users of the type.  That's a relatively small number (probably just 1) of places to check for correctness, vs. having to have logic to handle the padding bit in basically every operation on the type in addition to all the other configuration-explosions that are actually mandatory.
I definitely think the class needs to know at least of either the number of integral bits, or if this padding exists since the initial task I wanted it to do was handle conversion between unsigned and signed types while taking care of this padding.

I think a good solution would be to pass an additional semantics type such that we can still pass a literal APSInt to represent the underlying integer and know stuff like saturation and padding. I don't think it should represent the scale or width though since those are configured by the target and can have different values.

This semantic type I think would also help discern how to treat the underlying APSInt.


================
Comment at: include/clang/Basic/FixedPoint.h:45
+
+  FixedPointNumber extend(unsigned Width) const {
+    llvm::APSInt ValCpy = Val_;
----------------
ebevhan wrote:
> 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.
This makes sense. I had the extend and trunc methods so I could match the width of another fixed type when comparing, but if that's the only use for it, it makes sense to have a function that just accepts the width and scale.


================
Comment at: lib/AST/ASTContext.cpp:10320
+
+  if (DstWidth > SrcWidth) Val_ = Val_.extend(DstWidth);
+
----------------
ebevhan wrote:
> If you do rescaling before and after resizing, you can use `extOrTrunc` instead.
I think having this line just be `Val_ = Val_.extOrTrunc(DstWidth);` would allow for truncation to occur first before potentially right shifting.


Repository:
  rC Clang

https://reviews.llvm.org/D48661