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