rjmccall added inline comments.
================ Comment at: clang/include/clang/Basic/FixedPoint.h:67 + FixedPointSemantics + getCommonSemantics(const FixedPointSemantics &Other) const; + ---------------- leonardchan wrote: > rjmccall wrote: > > Actually representing the fully-precise value is operation-specific; you'd > > need a different calculation for at least addition and multiplication, and > > maybe also subtraction and (maybe?) division. Is this what you actually > > want? > For addition and subtraction, I believe we just need to extend and shift to a > type that is large enough to hold the larger scale and integral bits, then we > can do a normal add/sub. I think the relational operations can use this also > since we would just need to resize and align scales for them. > > For multiplication, the intrinsics we will use also assume the scale for both > operands are the same, which I believe will also require finding semantics > large enough to hold the larger scale and integral bits. > > ``` > declare iN @llvm.smul.fix(iN %L, %iN R, i32 %Scale) > ``` > > Division is similar since the saturating and non-saturating intrinsics assume > both operands have the same scale. > > ``` > declare iN @llvm.sdiv.fix(iN %L, %iN R, i32 %Scale) > ``` > > In each of these cases, I believe the common semantics just needs to be large > enough to hold the scale and largest representable value, which is what this > method does. Okay, so I believe what you're saying is that `getCommonSemantics` is defined as returning a semantics that is capable of precisely representing both input values, not one that's capable of precisely representing the result of the computation. The documentation could be clearer about that. ================ Comment at: clang/lib/Basic/FixedPoint.cpp:141 + if (ResultIsSigned || ResultHasUnsignedPadding) + CommonWidth++; + ---------------- leonardchan wrote: > rjmccall wrote: > > Is this right? If I have `a+b+c+d+e+f+g+h+i`, where those are all the same > > signed type, won't the width of the common semantics keep growing as I add > > more operands? > This should be right. Each addition is split into separate binary operations > which produce separate common fixed point semantics from 2 different > semantics. The width is also primarily made from the larger scale and large > number of integral bits (which excludes the sign or unsigned padding). > > For example: > > ``` > a+b -> Width: 32 (scale 14 + integral bits 16 + sign bit 1) > (a+b)+c -> Width: 32 ^^^^^ > ((a+b)+c)+d -> Width: 32 ^^^^^ > ``` > > Added a test for this. I see. If that's done consistently then it's fine that it technically slightly misrepresents the range of negative values. ================ Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3370 + + const auto *BinOp = dyn_cast<BinaryOperator>(op.E); + assert(BinOp && "Expected the operator to be a binary operator"); ---------------- Please use `cast`. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:1237 + if (Ty->isIntegerType()) + return 0; + ---------------- Please optimize this function around expecting a `BuiltinType`. That is, instead of separately checking for an integer type here, please assert it when (1) the `getAs` fails or (2) you fall into the `default` case. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:1304 + RHSTy = ResultTy; + } + ---------------- Hmm. So adding a signed integer to an unsigned fixed-point type always converts the integer to unsigned? That's a little weird, but only because the fixed-point rule seems to be the other way. Anyway, I assume it's not a bug in the spec. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53738/new/ https://reviews.llvm.org/D53738 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
