This patch adds codegen to Clang for fixed-point shift
operations.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/CodeGen/CGExprScalar.cpp | ||
---|---|---|
3515–3516 | Could this instead be: auto CommonFixedSema = IsShift ? LHSFixedSema : LHSFixedSema.getCommonSemantics(RHSFixedSema, true); | |
clang/test/Frontend/fixed_point_compound.c | ||
579 | 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. |
clang/lib/CodeGen/CGExprScalar.cpp | ||
---|---|---|
3515–3516 | 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. | |
clang/test/Frontend/fixed_point_compound.c | ||
579 | 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. |
clang/lib/CodeGen/CGExprScalar.cpp | ||
---|---|---|
3713 | I don't suppose you could file a bug for this and CC me on it so we can remember to do this sometime after this lands? | |
clang/test/Frontend/fixed_point_compound.c | ||
579 | Ah right I forgot that. | |
583–585 | I'm assuming these checks are also a result of D82663? I don't think for the padding case, we should need to do the compare and select if we're already using the signed sat intrinsic. But I'm guessing it might make the implementation more complicated by having to check for this. If this codegen comes from the EmitFixedPointConversion at the end of EmitFixedPointBinOp, perhaps it might be worthwhile adding a parameter in EmitFixedPointConversion to indicate that we shouldn't emit this. I think there's some cases in D82663 also where we might not need to do these checks afterwards with other operations like with: // UNSIGNED-NEXT: [[TMP25:%.*]] = call i16 @llvm.sadd.sat.i16(i16 [[TMP22]], i16 32767) // UNSIGNED-NEXT: [[TMP26:%.*]] = icmp slt i16 [[TMP25]], 0 // UNSIGNED-NEXT: [[SATMIN2:%.*]] = select i1 [[TMP26]], i16 0, i16 [[TMP25]] // UNSIGNED-NEXT: store i16 [[SATMIN2]], i16* @suf, align 2 |
clang/lib/CodeGen/CGExprScalar.cpp | ||
---|---|---|
3713 | I could do that. I guess the issue is larger than just this; I think other ops do not get proper UBSan checks for fixedpoint. So the ticket would probably entail supporting UBSan for fixedpoint, which is maybe a bit larger in scope. | |
clang/test/Frontend/fixed_point_compound.c | ||
583–585 | Yes, this is correct. And it is also correct that in the case of shift (and other operations) it is not necessary. It was just more elegant to make the common semantic signed and the result semantic unsigned and get the clamp automatically. I think the alternative is to handle the signedness of ops separate from the semantic during codegen and then emit a clamp manually per-operation instead of relying on the result conversion. |
Could this instead be: