[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1042
+        std::min(DstScale + DstFPSema.getIntegralBits(), ResultWidth));
+    Value *Zero = ConstantInt::get(CGF.getLLVMContext(), APInt(ResultWidth, 0));
+
----------------
You can just pass 0 here and it'll be zero-extended to the necessary width.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1044
+
+    Value *IsNegative = nullptr;
+    if (Mask != 0) {
----------------
I'm sorry, but this code is really impenetrable.  The variable names are non-descriptive, and there are a lot of uncommented dependencies between values, like how `IsNegative` propagates out, and like how it's checking without explanation that there's not a magnitude change using whether the mask ends up being all-zero.  Please just assign the two components of `ShouldCheckSaturation` to reasonably-named local variables and then use those to guide the code-generation here.

Also, the code being generated here is pretty weird.  I'm not sure the mask is helping; it might both produce better code and be easier to understand if you just broke it down into cases, like this:

```
  if a magnitude check is required {
    auto Max = maximum value of dest type;
    auto TooHigh = IsSigned ? Builder.CreateICmpSGT(Result, Max) : Builder.CreateICmpUGT(Result, Max);
    Result = Builder.CreateSelect(TooHigh, Max, Result);
  }

  if signed -> unsigned {
    auto Zero = zero value of dest type;
    Result = Builder.CreateSelect(Builder.CreateICmpSLT(Result, Zero), Zero, Result);
  } else if (IsSigned) {
    auto Min = minimum value of dest type;
    Result = Builder.CreateSelect(Builder.CreateICmpSLT(Result, Min), Min, Result);
  }
```


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1034
+  if (DstFPSema.isSaturated() &&
+      (CGF.getContext().getCorrespondingSaturatedType(SrcTy) != DstTy)) {
+    auto Mask = APInt::getBitsSetFrom(
----------------
leonardchan wrote:
> 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.
Wow, sorry for the edit failure in that review comment.  You're right, it should've been just (1) and the first (2).

Are there no fixed-point formats for which the range doesn't go up to (almost) 1?  I guess there probably aren't.


================
Comment at: lib/Sema/SemaExpr.cpp:5889
+    case Type::STK_MemberPointer:
+      llvm_unreachable("Unimplemented conversion from FixedPoint to type");
+    }
----------------
leonardchan wrote:
> 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.
You can still use `llvm_unreachable` for the cases here that aren't arithmetic types, namely all the pointer types.


Repository:
  rC Clang

https://reviews.llvm.org/D50616