[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1016
+  if (DstScale > SrcScale) {
+    // Need to allocate space before shifting left
+    ResultWidth = SrcWidth + DstScale - SrcScale;
----------------
In IR, this isn't really "allocating" space.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1034
+  if (DstFPSema.isSaturated() &&
+      (CGF.getContext().getCorrespondingSaturatedType(SrcTy) != DstTy)) {
+    auto Mask = APInt::getBitsSetFrom(
----------------
Why is this condition based on the formal types exactly matching rather than something about the FP semantics being different?  Formal types can correspond to the same format, right?

We need to check for saturation if we're either (1) decreasing the magnitude of the highest usable bit or (2) going signed->unsigned, (2) we're going signed->unsigned, or (3) we're going unsigned->signed without increasing the number of integral bits.  And I'd expect the checks we have to do in each case to be different.


================
Comment at: lib/Sema/SemaExpr.cpp:5889
+    case Type::STK_MemberPointer:
+      llvm_unreachable("Unimplemented conversion from FixedPoint to type");
+    }
----------------
Is there something I'm missing that actually diagnoses the unimplemented cases here?  There's a lot of code that seems to assume that any two arithmetic types can be converted to each other, and we do prefer not to crash the compiler, especially on valid code.


Repository:
  rC Clang

https://reviews.llvm.org/D50616