[PATCH] D57219: [Fixed Point Arithmetic] Fixed Point Comparisons

Mon Jan 28 16:20:51 PST 2019

leonardchan added inline comments.

================
Comment at: clang/test/Frontend/fixed_point_comparisons.c:56
+
+void TestComparisons() {
+  short _Accum sa;
----------------
ebevhan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > Missing saturating and saturating/non-saturating comparisons. I'd like to see the differences between unsigned padding and not there, if there are any.
> > I don't think there should be since we compare by converting to a common type that should fit both operands, but it does help to have tests that also confirm this. Added some saturation cases under `TestSaturationComparisons`.
> > 
> > As for padding, `TestSaturationComparisons` have cases comparing signed with unsigned types, and there are other cases in `TestComparisons` and `TestIntComparisons` that deal with unsigned comparisons. The way the lit tests are organized, lines marked with `CHECK` mean that those lines are produced for both the padding and no padding cases whereas `SIGNED` or `UNSIGNED` are produced exclusively for no padding and padding cases, respectively.
> There is a difference between saturating signed types and saturating unsigned types, though; the common type of two of the same saturating unsigned type is one bit less due to padding.
> 
> Unless there is something in the type commoning that I've missed?
You're right. It's just that when comparing values, we care about padding and not saturation since we aren't "writing" a value, so a comparison between a `_Sat short _Accum` and `_Sat unsigned short _Accum` should be the same as one for a `short _Accum` and `unsigned short _Accum`. With unsigned padding, both operations should not result in any resizing/scaling since they're already the same size and scale.

Added a test case for this under `TestSaturationComparisons`.

Repository:
  rC Clang

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

https://reviews.llvm.org/D57219