leonardchan marked an inline comment as done. leonardchan added inline comments.
================ 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 + ---------------- leonardchan wrote: > ebevhan wrote: > > leonardchan wrote: > > > 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. > > > 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. > > > > That's right. Though, for integer to fixed-point conversion, I don't think > > you ever need to upcast first; either the integer is larger than or equal > > to the integral part of the fixed-point type, in which case we can check > > the magnitude in the type as is, or it's smaller, and we don't have to do > > any saturation. > > > > > I think it might be simpler though if in `EmitFixedPointConversion`, we > > > treat an integer as a 'zero-scale fixed point number'. > > > > Isn't this already the case? The semantics for an integer type are fetched > > as a zero scale fixed-point type and used that way (except when the target > > is an integer due to the rounding requirement). > > That's right. Though, for integer to fixed-point conversion, I don't think > > you ever need to upcast first; either the integer is larger than or equal > > to the integral part of the fixed-point type, in which case we can check > > the magnitude in the type as is, or it's smaller, and we don't have to do > > any saturation. > > I see. I think this would be more of a matter then of when checking for > saturation, we either should check against the source value after resizing > and scaling (container), or the source by itself before resizing and scaling. > Actually, I think that when comparing saturation against the source value, we > could save an instruction since we can just generate a constant to compare > the source against instead of comparing against a potentially shifted value. > I think this could work when converting from fixed point types as well. > > With saturation conversion, (if the target scale >= src scale) currently we > could generate up to 7 instructions: > - 1 resize + 1 shift on the result if target scale > src scale > - 1 cmp gt + 1 select for clamping to max > - 1 cmp lt + 1 select for clamping to min > - 1 resize if container width != target width > > I think we don't need either the first or last resize if the constants that > we check against are the respective max's/min's of the target type against > the source. I think this deserves a patch on it's own though since it could > change a bunch of tests that depend on `EmitFixedPointConversion`. > > >Isn't this already the case? The semantics for an integer type are fetched > >as a zero scale fixed-point type and used that way (except when the target > >is an integer due to the rounding requirement). > > What I meant by this was that we are already doing the right thing in that we > calculate the result with the full precision of both operands. Added this change in D57553 and made it a child of this patch. 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
