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: > > > 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. > > I think the patch demonstrated to me that this emission can't be optimized > > in general without breaking the minmax pattern on the saturation, and that > > is very useful in some cases. > > > > What I think I'm concerned about is conversions like `int -> _Sat _Fract`, > > where there really is no point to upscaling at all, since the resulting > > will either be -1, 0 or ~0.999. > I think upscaling is also necessary in this case if we decide to keep the > minmax pattern for all conversion cases, otherwise we'd still be comparing > against the result: > > ``` > %0 = load i32, i32* %i, align 4 > %1 = icmp sgt i32 %0, 1 > %satmax = select i1 %1, i32 32767, i32 %0 > %2 = icmp slt i32 %0, 1 > %satmin = select i1 %2, i32 -32768, i32 %satmax > store i16 %resize1, i16* %sat_f, align 2 > ``` > > However, this takes only 6 instructions whereas the pattern we currently have > takes 9: > > ``` > %0 = load i32, i32* %i, align 4 > %resize = sext i32 %0 to i47 > %upscale = shl i47 %resize, 15 > %1 = icmp sgt i47 %upscale, 32767 > %satmax = select i1 %1, i47 32767, i47 %upscale > %2 = icmp slt i47 %satmax, -32768 > %satmin = select i1 %2, i47 -32768, i47 %satmax > %resize1 = trunc i47 %satmin to i16 > store i16 %resize1, i16* %sat_f, align 2 > ``` > > We could say that for specific cases, we compare against the source value and > others we do minmax. Woops, the first example should have `i16`s in the `select` instructions, not `i32`s. 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
