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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


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


================
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]])
----------------
ebevhan wrote:
> 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.
Ah right I forgot that.


================
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
----------------
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
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83294