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