ebevhan marked an inline comment as done. ebevhan added inline comments.
================ Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3603-3604 auto ResultFixedSema = Ctx.getFixedPointSemantics(ResultTy); - auto CommonFixedSema = LHSFixedSema.getCommonSemantics(RHSFixedSema, true); + auto CommonFixedSema = LHSFixedSema.getCommonSemantics( + IsShift ? LHSFixedSema : RHSFixedSema, true); ---------------- leonardchan wrote: > Could this instead be: > > ``` > auto CommonFixedSema = IsShift ? LHSFixedSema : > LHSFixedSema.getCommonSemantics(RHSFixedSema, true); > ``` > > In theory, yes, but I'm sort of piggybacking off of D82663, and for the signedness to be correct we need to get the 'common' semantic even in the shift case. ================ Comment at: clang/test/Frontend/fixed_point_compound.c:384 + // CHECK-NEXT: [[TMP4:%.*]] = load i16, i16* @suf, align 2 + // CHECK-NEXT: [[TMP5:%.*]] = trunc i32 [[TMP3]] to i16 + // SIGNED-NEXT: [[TMP6:%.*]] = call i16 @llvm.ushl.sat.i16(i16 [[TMP4]], i16 [[TMP5]]) ---------------- leonardchan wrote: > This seems very unlikely, but if `i` in this case was a value at least 2^16, > the upper half would be cut off and we'd potentially shift by an improper > amount for some values of `i` when we should actually clamp to the max value. > We should probably still find the common semantics for both sides if we're > doing a shift. If the value is so large to be cut off by that truncation then the value is greater than the bitwidth, which is UB. So I don't think it's an issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83294/new/ https://reviews.llvm.org/D83294 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits