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