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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
