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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

Sorry for the delay in responding to this. Been working on a few other patches.

A couple of the bigger changes so far:

- Added `getCommonSemantics` to `FixedPointSemantics` for finding a common full precision semantic.
- `getFixedPointSemantics` accepts an int type.
- `FixedPointConversions` was renamed to `handleFixedPointConversions` and is a part of `UsualArithmeticConversions`
- `EmitFixedPointAdd` now works by converting both operands to a common `FixedPointSemantics` that can hold the full precision of the result, performing a regular `add` or call to the addition intrinsics, and converting back to the result type if necessary.
  - This also fixed the case with calling the sat add intrinsics.
- Change `EmitFixedPointConversion` to accept `FixedPointSemantics`. I kept the original one still which takes `QualTypes` that are fixed point or integer types and gets the `FixedPointSemantics` from them.

The only thing I didn't include in this patch were the suggested changes to `FixedPointSemantics` which would indicate if the semantics were original from an integer type.  I'm not sure how necessary this is because AFAIK the only time we would need to care if the semantics came from an int is during conversion from a fixed point type to an int, which should only occur during casting and not binary operations. In that sense, I don't think this info needs to be something bound to the semantics, but more to the conversion operation. I also don't think that would be relevant for this patch since all integers get converted to fixed point types anyway and not the other way around.



================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3208
+  if (LHSWidth < CommonWidth)
+    LHS = LHSSign ? Builder.CreateSExt(LHS, CommonTy)
+                  : Builder.CreateZExt(LHS, CommonTy);
----------------
bjope wrote:
> `LHS = Builder.CreateIntCast(LHS, CommonTy, LHSSign);`
> 
> (I think that it even is possible to remove the condition and always do this (the cast will be a no-op if LHSWidth==CommonWidth))
Replaced this part with converting the operands to common semantics, then regular addition.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3211
+  if (RHSWidth < CommonWidth)
+    RHS = RHSSign ? Builder.CreateSExt(RHS, CommonTy)
+                  : Builder.CreateZExt(RHS, CommonTy);
----------------
bjope wrote:
> Same as above, this can be simplified as:
>   RHS = Builder.CreateIntCast(RHS, CommonTy, RHSSign);
> 
Same as above.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3214
+
+  // Align scales
+  unsigned CommonScale = std::max({LHSScale, RHSScale, ResultScale});
----------------
bjope wrote:
> Arithmetically I think that it would be enough to align the scale to
> ```
>  std::max(std::min(LHSScale, RHSScale), ResultScale)
> ```
> As adding low fractional bits outside the result precision, when we know that those bits are zero in either of the inputs, won't impact any more significant bits in the result.
> 
> Maybe your solution is easier and good enough for a first implementation. And maybe `opt` is smart enough to optimize this anyway. Or maybe codegen could be worse due to not using legal types. Or maybe codegen could be improved due to using sadd.sat in more cases?
> Lots of "maybes", so I don't think you need to do anything about this right now. It is just an idea from my side.
Same as above.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3223-3247
+    if (ResultWidth < CommonWidth) {
+      // In the event we extended the sign of both operands, the intrinsic will
+      // not saturate to the initial bit width of the result type. In this case,
+      // we can default to use min/max clamping. This can arise from adding an
+      // operand of an int type whose width is larger than the width of the
+      // other fixed point operand.
+      // If we end up implementing the intrinsics for saturating ints to
----------------
leonardchan wrote:
> There is a case when we cannot use the sadd/uadd intrinsics because those intrinsics saturate to the width of the operands when instead we want to saturate to the width of the resulting fixed point type. The problem is that if we are given operands with different scales that require extending then shifting before adding, the bit width no longer matches that of the resulting saturated type, so the sat intrinsics instead saturate to the extended width instead of the result type width. This could be a problem with my logic though and there could be a better way to implement addition.
> 
> Otherwise, as far as I can tell, this could be addressed by replacing the clamping with an intrinsic, which would create a use case for the signed/unsigned saturation intrinsics. Alternatively, the addition intrinsics could be expanded to also provide another argument that contains a desired saturation bit width, which would allow for collapsing the saturation type case into a call to this one intrinsic function. 
> 
Fixed by converting operands to common fixed point semantics, adding, then converting the result. This ends up producing the same instructions as the original tests. 


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1393
+/// rules in N1169 4.1.4.
+QualType Sema::FixedPointConversions(ExprResult &FixedExpr,
+                                     ExprResult &OtherExpr, bool IsCompAssign) {
----------------
ebevhan wrote:
> I'm not sure I understand why this is in a separate function. What part of this doesn't work in UsualArithmeticConversions, in a 'handleFixedPointConversion' similar to the other cases?
I might've misinterpreted the standard in that operations between fixed point and other fixed point or integers didn't fall under usual arithmetic conversions, but I can see how it would be simpler to keep in in UAC since almost all checks on binary operations call this.

Added a changed `FixedPointConversions` to `handleFixedPointConversion`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1416
+  // converted to its corresponding signed fixed-point type and the resulting
+  // type is the type of the converted operand.
+  if (OtherTy->isSignedFixedPointType() &&
----------------
ebevhan wrote:
> I feel like this logic can be folded into the rank handling. Does it work properly if you give signed types higher rank than unsigned ones?
I think this is necessary since it seems the purpose of this is to align the scales between signed and unsigned types if they are different. For signed and unsigned types specifically, the standard also says `unsigned fixed-point operand is converted to its corresponding signed fixed-point type` which I think means it's an actual conversion.


Repository:
  rC Clang

https://reviews.llvm.org/D53738