[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:3603-3604
   auto ResultFixedSema = Ctx.getFixedPointSemantics(ResultTy);
-  auto CommonFixedSema = LHSFixedSema.getCommonSemantics(RHSFixedSema, true);
+  auto CommonFixedSema = LHSFixedSema.getCommonSemantics(
+      IsShift ? LHSFixedSema : RHSFixedSema, true);
 
----------------
Could this instead be:

```
auto CommonFixedSema = IsShift ? LHSFixedSema : LHSFixedSema.getCommonSemantics(RHSFixedSema, true);
```




================
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]])
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83294