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