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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
