[PATCH] D83294: [Fixed Point] Add codegen for fixed-point shifts.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked an inline comment as done.
ebevhan added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3857
+
+  // TODO: This misses out on the sanitizer check below.
+  if (Ops.isFixedPointOp())
----------------
leonardchan wrote:
> 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?
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.


================
Comment at: clang/test/Frontend/fixed_point_compound.c:388-390
+  // UNSIGNED-NEXT: [[TMP7:%.*]] = icmp slt i16 [[TMP6]], 0
+  // UNSIGNED-NEXT: [[SATMIN:%.*]] = select i1 [[TMP7]], i16 0, i16 [[TMP6]]
+  // UNSIGNED-NEXT: store i16 [[SATMIN]], i16* @suf, align 2
----------------
leonardchan wrote:
> 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
> ```
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83294/new/

https://reviews.llvm.org/D83294