[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I'd be interested in seeing tests for two saturating unsigned _Fract with and without padding.

If the padding case emits a uadd_sat, that seems wrong; uadd_sat doesn't saturate on the padding bit, but will saturate the whole number, which can result in invalid representation both with or without saturation taking effect.

Common semantics for unsigned types with padding might need a bit of trickery to get it to do the right thing.



================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3383
+
+  // onvert the operands to the full precision type.
+  Value *FullLHS = EmitFixedPointConversion(LHS, LHSFixedSema, CommonFixedSema,
----------------
Missing a C there.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1306
+    OtherTy = ResultTy;
+  }
+
----------------
leonardchan wrote:
> ebevhan wrote:
> > Does this handle compound assignment? The other functions handle this specially.
> Currently no. I was initially intending to add addition compound assignment in a different patch, but do you think it would be appropriate to include it here? I imagine the changes to Sema wouldn't be difficult, but would probably require adding a lot more tests. 
That's fine by me, then. Take it in the next patch.


================
Comment at: clang/test/Frontend/fixed_point_add.c:36
+
+  // To smaller scale and same width
+  // CHECK:      [[SA:%[0-9]+]] = load i16, i16* %sa, align 2
----------------
What do these comments refer to? The scale is the same for both operands here.


Repository:
  rC Clang

https://reviews.llvm.org/D53738