leonardchan added inline comments.
================
Comment at: lib/Basic/FixedPoint.cpp:40
+ if (DstWidth > Val.getBitWidth())
+ Val = Val.extend(DstWidth);
+ if (Upscaling)
----------------
ebevhan wrote:
> It should be possible to replace this with `extOrTrunc` and move it below the
> saturation.
I don't think we can move the extension below saturation since saturation
requires checking the bits above the dst width which may require extending and
shifting beforehand.
Still leaving it above saturation may require checking for the width over using
`extOrTrunc` to prevent truncating early before right shifting.
================
Comment at: lib/Basic/FixedPoint.cpp:58
+ if (!DstSema.isSigned && Val.isNegative()) {
+ Val = 0;
+ } else if (!(Masked == Mask || Masked == 0)) {
----------------
ebevhan wrote:
> 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?
You're right. I did not test for this since I thought this would fall under
undefined behavior converting a signed negative number to an unsigned number.
I'll add a check for saturation since I imagine most people would expect
modular wrap around, but do you think I should add a test case for this?
================
Comment at: lib/Basic/FixedPoint.cpp:73
+ if (DstWidth < Val.getBitWidth())
+ Val = Val.trunc(DstWidth);
+
----------------
ebevhan wrote:
> When we say 'width' do we mean the width - padding or the width including
> padding? What information does the semantics structure enshrine?
This width includes the padding. This is commented on over `struct
fixedPointSema` where this width represents that of the underlying APInt, so if
the sema is unsigned and hasUnsignedPadding is true, then in a provided APInt
with this width, the `width-1` bit is padding.
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
