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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to