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

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

> 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.



> For the integer conversion though, I was going to add that in a separate fixed-point-to-int and int-to-fixed-point patch.

Okay, that's fine! You're right in that the semantics commoning will never produce an integer semantic like that.



================
Comment at: clang/lib/Basic/FixedPoint.cpp:129
+      std::max(NonFractBits, OtherNonFractBits) + CommonScale;
+
+  bool ResultIsSigned = isSigned() || Other.isSigned();
----------------
Does this properly compensate for types of equal width but different signedness?

If you have a signed 8-bit 7-scale fixed point number, and operate with an unsigned 8-bit integer, you'll get CommonWidth=15 and CommonScale=7, leaving 8 bits of integral. But the MSB in that cannot both be sign bit and unsigned MSB at the same time. I think you need an extra bit.


================
Comment at: clang/lib/Basic/FixedPoint.cpp:135
+    ResultHasUnsignedPadding =
+        hasUnsignedPadding() || Other.hasUnsignedPadding();
+
----------------
If one has padding but the other doesn't, then the padding must be significant, so the full precision semantic cannot have padding.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3385
+  if (IsCommonSaturated)
+    CommonFixedSema.setSaturated(false);
+
----------------
Can EmitFixedPointConversion not determine this on its own?


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3387
+
+  // Convert and align the operands.
+  Value *LHSAligned = EmitFixedPointConversion(
----------------
'align' usually means something else. Maybe 'Convert the operands to the full precision type' and 'FullLHS'?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1306
+    OtherTy = ResultTy;
+  }
+
----------------
Does this handle compound assignment? The other functions handle this specially.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:1403
+  else if (RHSType->isFixedPointType())
+    return handleFixedPointConversion(*this, RHS, LHS, RHSType, LHSType);
+
----------------
Can this commutation be folded into the function to align it with the rest?


Repository:
  rC Clang

https://reviews.llvm.org/D53738