[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1034
+  if (DstFPSema.isSaturated() &&
+      (CGF.getContext().getCorrespondingSaturatedType(SrcTy) != DstTy)) {
+    auto Mask = APInt::getBitsSetFrom(
----------------
rjmccall wrote:
> 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.
No problem. The smallest range that a fixed point type can cover is the `_Fract` type which covers [-1, 1).


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1044
+
+    Value *IsNegative = nullptr;
+    if (Mask != 0) {
----------------
rjmccall wrote:
> 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);
>   }
> ```
My bad. I guess it seemed intuitive at first but can see how it's difficult to read.

This is the full logic and reasoning for the mask: https://reviews.llvm.org/D48661?id=153426#inline-427647

But to summarize, it's essentially for checking if the bits above the MSB changed which would indicate saturation needs to occur.
- `Mask` is for getting the bits above the integral bits in the resulting type (for signed types, this also captures the sign bit)
- `Masked` is the bits above the highest integral bit in the resulting type
- `Masked == Mask` indicates that all the bits above the highest integral bit were ones (the value is negative) and none of them changed (no change in magnitude)
- `Mask == 0` indicates that all the bits above the highest integral bit were zeros (the value is non-negative) and none of them changed (no  change in magnitude)
- `Masked == Mask || Mask == 0` therefore indicates no change in magnitude
- `!(Masked == Mask || Mask == 0)` indicates a change in magnitude and we should saturate (but to save an extra instruction, this was emitted as `Masked != Mask && Mask != 0`
- If we saturate, `Mask` also happens to be the min value we saturate to for signed types and `~Mask` is also the max value we saturate to.

The reason for the `IsNegative` laid out like that is to prevent from emitting an extra instruction for checking a negative value.


Repository:
  rC Clang

https://reviews.llvm.org/D50616