ebevhan added a comment. > The only thing I didn't include in this patch were the suggested changes to > FixedPointSemantics which would indicate if the semantics were original from > an integer type. I'm not sure how necessary this is because AFAIK the only > time we would need to care if the semantics came from an int is during > conversion from a fixed point type to an int, which should only occur during > casting and not binary operations. In that sense, I don't think this info > needs to be something bound to the semantics, but more to the conversion > operation. I also don't think that would be relevant for this patch since all > integers get converted to fixed point types anyway and not the other way > around.
> For the integer conversion though, I was going to add that in a separate > fixed-point-to-int and int-to-fixed-point patch. Okay, that's fine! You're right in that the semantics commoning will never produce an integer semantic like that. ================ Comment at: clang/lib/Basic/FixedPoint.cpp:129 + std::max(NonFractBits, OtherNonFractBits) + CommonScale; + + bool ResultIsSigned = isSigned() || Other.isSigned(); ---------------- Does this properly compensate for types of equal width but different signedness? If you have a signed 8-bit 7-scale fixed point number, and operate with an unsigned 8-bit integer, you'll get CommonWidth=15 and CommonScale=7, leaving 8 bits of integral. But the MSB in that cannot both be sign bit and unsigned MSB at the same time. I think you need an extra bit. ================ Comment at: clang/lib/Basic/FixedPoint.cpp:135 + ResultHasUnsignedPadding = + hasUnsignedPadding() || Other.hasUnsignedPadding(); + ---------------- If one has padding but the other doesn't, then the padding must be significant, so the full precision semantic cannot have padding. ================ Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3385 + if (IsCommonSaturated) + CommonFixedSema.setSaturated(false); + ---------------- Can EmitFixedPointConversion not determine this on its own? ================ Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3387 + + // Convert and align the operands. + Value *LHSAligned = EmitFixedPointConversion( ---------------- 'align' usually means something else. Maybe 'Convert the operands to the full precision type' and 'FullLHS'? ================ Comment at: clang/lib/Sema/SemaExpr.cpp:1306 + OtherTy = ResultTy; + } + ---------------- Does this handle compound assignment? The other functions handle this specially. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:1403 + else if (RHSType->isFixedPointType()) + return handleFixedPointConversion(*this, RHS, LHS, RHSType, LHSType); + ---------------- Can this commutation be folded into the function to align it with the rest? Repository: rC Clang https://reviews.llvm.org/D53738 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
