leonardchan abandoned this revision. leonardchan marked an inline comment as done. leonardchan added a comment.
In D57553#1388493 <https://reviews.llvm.org/D57553#1388493>, @ebevhan wrote: > In D57553#1381920 <https://reviews.llvm.org/D57553#1381920>, @leonardchan > wrote: > > > In regards to solving the problem of resizing for int conversions, I'm > > starting to think that we will need that initial resize since if we want to > > retain the min-max pattern for all conversions, then it would be necessary > > to extend and shift when converting from an int to fixed point. Otherwise, > > I think we'd have the initial pattern I proposed where we check against the > > source value, but not have this pattern. > > > Yes, I'm starting to think so too. It's just not possible to not resize and > keep the minmax pattern at the same time. We can't upshift without resizing > first (because any bits above the scale can affect saturation), and if we > wanted to saturate purely on the non-rescaled value, then the (mandatory) > rescaling after saturation could destroy the possibly saturated result (since > it would shift in zeroes from the right if upscaling, which breaks the result > if it saturated to max). > > Sorry for going down this path, that was rather pointless. No problem. Abandoning this patch. ================ Comment at: clang/test/Frontend/fixed_point_conversions.c:194 + // DEFAULT-NEXT: [[RESULT2:%[0-9a-z]+]] = select i1 [[USE_MIN]], i32 -2147483648, i32 [[RESULT]] + // DEFAULT-NEXT: store i32 [[RESULT2]], i32* %sat_lf, align 4 ---------------- ebevhan wrote: > This seems off. We're upshifting, then saturating on the upshifted value. But > the top bits could have been shifted out, so the result might not be correct. > > We can't upshift before saturating if the upshift could shift out bits. > > If the upshift is moved after the saturation, then the saturation no longer > works since it would be upshifting zeros into the saturation result, which is > also wrong. Ah you're right. I completely missed this. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57553/new/ https://reviews.llvm.org/D57553 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
