leonardchan added a comment.
Oh I forgot to submit the comments.
================
Comment at: clang/lib/AST/ExprConstant.cpp:9825
+ if (Result.isSigned() && !DstSigned) {
+ Overflow = Result < 0 || Result.ugt(DstMax);
+ } else if (Result.isUnsigned() && DstSigned) {
----------------
ebevhan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > The `Result < 0` is more clearly expressed as `Result.isNegative()` I
> > > think.
> > Ah, so I ran into something similar with the patch preceding this in
> > `APFixedPoint::convert()`. `isNegative()` is a method of `APInt` which
> > doesn't care about signage. It just checks if the last bit is set. `Result
> > < 0` calls `APSInt::operator<()` which cares about the sign and checks if
> > this signed int is less than zero.
> >
> > have no big problem with this, but if `Result.isNegative()` is more
> > readable, I could also add `Result.isSigned()` which together effectively
> > does the same thing as `Result < 0`.
> This makes sense, but you're already checking if the value is signed in the
> line above, so it shouldn't be an issue.
Ah, forgot about that.
================
Comment at: clang/test/Frontend/fixed_point_conversions.c:426
+ _Sat short _Accum sat_sa;
+ _Sat unsigned short _Accum sat_usa;
+
----------------
ebevhan wrote:
> There are no tests here for what you get if you convert an integer to a
> fixed-point type with a larger integral part than the integer has.
Added to the end of `TestIntToFixedPoint`
================
Comment at: clang/test/Frontend/fixed_point_conversions.c:437
+ // DEFAULT-NEXT: [[RES:%[a-z0-9]+]] = trunc i39 [[SATMIN]] to i16
+ // DEFAULT-NEXT: store i16 [[RES]], i16* %sat_sa, align 2
+
----------------
ebevhan wrote:
> Conversions like this are a bit odd. There shouldn't be a need to
> upsize/upscale the container before the saturation, should there?
I see. You're saying that we can just check directly if the value exceeds 255
(or goes under -256) since the range of target values can always fit in the
range of source values. Therefore we do not need to cast up since the only
reason we would need to is if converting to a type with a greater source range.
In this case, we could technically have a special case for integers where I
think we can perform the saturation checks without the initial sign extension.
I think it might be simpler though if in `EmitFixedPointConversion`, we treat
an integer as a 'zero-scale fixed point number'. I think the reason the
upsizing occurs in the first place is so that we satisfy the first FX
conversion rule of calculating the result with full precision of both operands.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56900/new/
https://reviews.llvm.org/D56900
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
