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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83294