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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: clang/include/clang/Basic/FixedPoint.h:67
+  FixedPointSemantics
+  getCommonSemantics(const FixedPointSemantics &Other) const;
+
----------------
rjmccall wrote:
> Actually representing the fully-precise value is operation-specific; you'd need a different calculation for at least addition and multiplication, and maybe also subtraction and (maybe?) division.  Is this what you actually want?
For addition and subtraction, I believe we just need to extend and shift to a type that is large enough to hold the larger scale and integral bits, then we can do a normal add/sub. I think the relational operations can use this also since we would just need to resize and align scales for them.

For multiplication, the intrinsics we will use also assume the scale for both operands are the same, which I believe will also require finding semantics large enough to hold the larger scale and integral bits.

```
declare iN @llvm.smul.fix(iN %L, %iN R, i32 %Scale)
```

Division is similar since the saturating and non-saturating intrinsics assume both operands have the same scale.

```
declare iN @llvm.sdiv.fix(iN %L, %iN R, i32 %Scale)
```

In each of these cases, I believe the common semantics just needs to be large enough to hold the scale and largest representable value, which is what this method does.


================
Comment at: clang/lib/AST/ASTContext.cpp:10489
+    return FixedPointSemantics::GetIntegerSemantics(getIntWidth(Ty),
+                                                    Ty->isSignedIntegerType());
+
----------------
rjmccall wrote:
> Does `getIntWidth` do the thing you want for `bool`?
I think so. The resulting semantics for this should be unsigned with a width of 1. I believe a `_Bool` currently never gets passed to this since `UsualUnaryConversions` casts a `_Bool` into an `int`.

Currently adding a fixed point type with a bool is treated the same as adding with an int. Added a test for this also.


================
Comment at: clang/lib/Basic/FixedPoint.cpp:141
+  if (ResultIsSigned || ResultHasUnsignedPadding)
+    CommonWidth++;
+
----------------
rjmccall wrote:
> Is this right?  If I have `a+b+c+d+e+f+g+h+i`, where those are all the same signed type, won't the width of the common semantics keep growing as I add more operands?
This should be right. Each addition is split into separate binary operations which produce separate common fixed point semantics from 2 different semantics. The width is also primarily made from the larger scale and large number of integral bits (which excludes the sign or unsigned padding).

For example:

```
a+b -> Width: 32 (scale 14 + integral bits 16 + sign bit 1)
(a+b)+c -> Width: 32 ^^^^^
((a+b)+c)+d -> Width: 32 ^^^^^
```

Added a test for this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738