[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


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


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1034
+  if (DstFPSema.isSaturated() &&
+      (CGF.getContext().getCorrespondingSaturatedType(SrcTy) != DstTy)) {
+    auto Mask = APInt::getBitsSetFrom(
----------------
rjmccall wrote:
> 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.
For simplicity, I more or less copied the logic from `APFixedPoint::convert()` which performs a saturation check that covers all of these cases if the destination semantics were saturated.

Added another condition that checks if we need to perform saturation checks. I think your (1) and (3) might be the same thing since I think we only really need to check if the magnitude decreases or if going from signed -> unsigned. 

I think though that the IR emission would be the same since both cases will require checking for a change in the magnitude (via the mask). The only difference is that if going from signed->unsigned, the min saturation is zero if the value is negative.


================
Comment at: lib/Sema/SemaExpr.cpp:5889
+    case Type::STK_MemberPointer:
+      llvm_unreachable("Unimplemented conversion from FixedPoint to type");
+    }
----------------
rjmccall wrote:
> 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.
The plan was to implement these in other patches. I wasn't sure if `llvm_unreachable()` was ok to use as a placeholder for features that will eventually be implemented.

Added diagnostics here for now to be eventually removed once these casts are implemented in later patches.


Repository:
  rC Clang

https://reviews.llvm.org/D50616