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 + ---------------- 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. 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
