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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/Basic/FixedPoint.h:67
+  FixedPointSemantics
+  getCommonSemantics(const FixedPointSemantics &Other) const;
+
----------------
leonardchan wrote:
> 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.
Okay, so I believe what you're saying is that `getCommonSemantics` is defined as returning a semantics that is capable of precisely representing both input values, not one that's capable of precisely representing the result of the computation.  The documentation could be clearer about that.


================
Comment at: clang/lib/Basic/FixedPoint.cpp:141
+  if (ResultIsSigned || ResultHasUnsignedPadding)
+    CommonWidth++;
+
----------------
leonardchan wrote:
> 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.
I see.  If that's done consistently then it's fine that it technically slightly misrepresents the range of negative values.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3370
+
+  const auto *BinOp = dyn_cast<BinaryOperator>(op.E);
+  assert(BinOp && "Expected the operator to be a binary operator");
----------------
Please use `cast`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1237
+  if (Ty->isIntegerType())
+    return 0;
+
----------------
Please optimize this function around expecting a `BuiltinType`.  That is, instead of separately checking for an integer type here, please assert it when (1) the `getAs` fails or (2) you fall into the `default` case.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+    RHSTy = ResultTy;
+  }
+
----------------
Hmm.  So adding a signed integer to an unsigned fixed-point type always converts the integer to unsigned?  That's a little weird, but only because the fixed-point rule seems to be the other way.  Anyway, I assume it's not a bug in the spec.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738