ebevhan added inline comments.
================
Comment at: include/clang/Basic/FixedPoint.h:67
+ // Convert this number to match the semantics provided.
+ void convert(const struct fixedPointSemantics &DstSema);
+
----------------
If this class is supposed to be used like APInt and APSInt, perhaps this
function should return a copy instead of modifying in place.
The reason that the APFloat `convert` function modifies in place is because it
needs to return a status code, but I don't think that is necessary for this.
================
Comment at: lib/Basic/FixedPoint.cpp:53
+ // We can overflow here
+ unsigned ShiftAmt = DstScale - Scale;
+ if (Val < 0 && Val.countLeadingOnes() >= ShiftAmt)
----------------
leonardchan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > ebevhan wrote:
> > > > I think saturation can be modeled a bit better. It should be possible
> > > > to do the overflow check/saturation once instead of twice, and also
> > > > have it fit in better with the other conversions.
> > > >
> > > > Saturation on a value is essentially checking whether all bits above
> > > > and including a certain bit are all the same, and 'clamping' to either
> > > > the minimum (the mask) or maximum (inverse of the mask) if not.
> > > > ```
> > > > // Saturate Value to SatWidth. This will clamp Value at the signed
> > > > min/max of a value that is SatWidth long.
> > > > Saturate(SatWidth):
> > > > Mask = highBits(Width, SatWidth + 1)
> > > > Masked = Value & Mask
> > > > Result = Value
> > > > if (Masked == Mask || Masked == 0) {
> > > > if (Masked.isNegative())
> > > > Result = Mask
> > > > else
> > > > Result = ~Mask
> > > > }
> > > > ```
> > > > This cannot be done on the value in the source scale, since upscaling
> > > > after clamping would produce an incorrect value - we would shift in 0's
> > > > from the right even though we should have saturated all bits
> > > > completely. Also, we cannot upscale without first extending or we might
> > > > shift out bits on the left that could affect saturation.
> > > >
> > > > One thing to note is that (I'm ***pretty sure*** this is the case)
> > > > fractional bits cannot affect saturation. This means that we can find a
> > > > common scale, then perform the saturation operation, and then resize,
> > > > and the value should just fall into place. Basically:
> > > > # Upscale if necessary, but extend first to ensure that we don't drop
> > > > any bits on the left. Do this by resizing to `SrcWidth - SrcScale +
> > > > std::max(SrcScale, DstScale)` and then upscaling normally by `DstScale
> > > > - SrcScale`.
> > > > # Downscale if necessary by `SrcScale - DstScale`. (I think; in our
> > > > downstream, we saturate first and then downscale, but we also assume
> > > > that saturation can only occur on _Fract, and doing saturation first
> > > > makes the saturation width calculation a bit messier because it will be
> > > > a `max` expression. I'm unsure if the order can be changed.)
> > > > # At this point, the scale of the value should be `DstScale`.
> > > > Saturate with `Saturate(DstWidth)`.
> > > > # Now the value should be in range for the new width, and will be at
> > > > the right scale as well. Resize with `extOrTrunc(DstWidth)`.
> > > > # (Also; if the result was negative and the dest type is unsigned,
> > > > just make the result zero here instead.)
> > > >
> > > > Note that the order of operations is different than what I've mentioned
> > > > before with non-saturated conversion (downscale if needed, resize,
> > > > upscale if needed), but it works because we only do the upscale as a
> > > > resize+upscale. Somewhere in here you also need to change the
> > > > signedness of the value, but I'm not entirely certain where. Likely
> > > > after 4.
> > > >
> > > > Everything I've mentioned here is semi-conjectural, since our
> > > > implementation has different type widths than the defaults in this one,
> > > > we can only saturate on _Fract, and our unsigned types have padding.
> > > > It's possible that there are some assumptions that cause this method to
> > > > fail for unsigned without padding, or perhaps for some other type
> > > > conversion, but I haven't come up with a counterexample yet.
> > > Now that I think about it a bit more, it's clear that the Saturate
> > > routine does not work for unsigned fixed-point without padding. That
> > > would need to be taken into consideration, but the rest should work.
> > Thanks for the deeper explanation. The implementation is essentially the
> > same and catching the unsigned fixed point without padding is just checking
> > if the value is negative and we are casting to an unsigned.
> Also I wasn't sure if this was a typo, but I have
>
> ```
> if (!(Masked == Mask || Masked == 0)) {
> ```
>
> instead of
>
> ```
> if (Masked == Mask || Masked == 0) {
> ```
>
> because I think the mask comparison checks if the bits above the sign changed
> which indicates saturation.
Yes, that was a typo! Sorry! Good thing you realized it.
================
Comment at: lib/Basic/FixedPoint.cpp:25
+
+ // Exit early
+ if (getWidth() == DstWidth && getScale() == DstScale &&
----------------
Why the early exit? What does the logic below not handle?
If this value has padding but the other doesn't (or vice versa), the scales
will be different and the code below should work just fine.
================
Comment at: lib/Basic/FixedPoint.cpp:40
+ if (DstWidth > Val.getBitWidth())
+ Val = Val.extend(DstWidth);
+ if (Upscaling)
----------------
It should be possible to replace this with `extOrTrunc` and move it below the
saturation.
================
Comment at: lib/Basic/FixedPoint.cpp:44
+ std::max(getScale(), DstScale));
+
+ if (Upscaling) {
----------------
Combine the above block with the below one.
================
Comment at: lib/Basic/FixedPoint.cpp:49
+ if (isSigned())
+ Val = Val.ashr(getScale() - DstScale);
+ else
----------------
`Val` is an APSInt so it should be possible to just do `>>` and get the right
behavior, provided that the signedness is correct.
================
Comment at: lib/Basic/FixedPoint.cpp:58
+ if (!DstSema.isSigned && Val.isNegative()) {
+ Val = 0;
+ } else if (!(Masked == Mask || Masked == 0)) {
----------------
If the value is unsigned and negative, we always zero it?
Also, this code always saturates the value regardless of whether the
destination semantics call for it, no?
================
Comment at: lib/Basic/FixedPoint.cpp:73
+ if (DstWidth < Val.getBitWidth())
+ Val = Val.trunc(DstWidth);
+
----------------
When we say 'width' do we mean the width - padding or the width including
padding? What information does the semantics structure enshrine?
================
Comment at: lib/Basic/FixedPoint.cpp:93
+ if (getWidth() < CommonWidth)
+ ThisVal = ThisVal.extend(CommonWidth);
+ if (OtherWidth < CommonWidth)
----------------
Use `extOrTrunc` and you won't need the ifs.
All of those accessors are a bit misnamed, it should be called
`extOrTruncOrSelf`...
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
