Page MenuHomePhabricator

[Fixed Point] Add codegen for fixed-point shifts.
Needs ReviewPublic

Authored by ebevhan on Jul 7 2020, 4:18 AM.

Details

Summary

This patch adds codegen to Clang for fixed-point shift
operations.

Diff Detail

Event Timeline

ebevhan created this revision.Jul 7 2020, 4:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2020, 4:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
leonardchan added inline comments.Jul 8 2020, 2:10 PM
clang/lib/CodeGen/CGExprScalar.cpp
3603–3604

Could this instead be:

auto CommonFixedSema = IsShift ? LHSFixedSema : LHSFixedSema.getCommonSemantics(RHSFixedSema, true);
clang/test/Frontend/fixed_point_compound.c
384

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.

ebevhan marked an inline comment as done.Jul 9 2020, 4:03 AM
ebevhan added inline comments.
clang/lib/CodeGen/CGExprScalar.cpp
3603–3604

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
384

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.

leonardchan added inline comments.Jul 9 2020, 12:25 PM
clang/lib/CodeGen/CGExprScalar.cpp
3857

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
384

Ah right I forgot that.

388–390

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
ebevhan marked an inline comment as done.Jul 10 2020, 1:42 AM
ebevhan added inline comments.
clang/lib/CodeGen/CGExprScalar.cpp
3857

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
388–390

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.