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