leonardchan added a comment.
Sorry for the delay in responding to this. Been working on a few other patches.
A couple of the bigger changes so far:
- Added `getCommonSemantics` to `FixedPointSemantics` for finding a common full
precision semantic.
- `getFixedPointSemantics` accepts an int type.
- `FixedPointConversions` was renamed to `handleFixedPointConversions` and is a
part of `UsualArithmeticConversions`
- `EmitFixedPointAdd` now works by converting both operands to a common
`FixedPointSemantics` that can hold the full precision of the result,
performing a regular `add` or call to the addition intrinsics, and converting
back to the result type if necessary.
- This also fixed the case with calling the sat add intrinsics.
- Change `EmitFixedPointConversion` to accept `FixedPointSemantics`. I kept the
original one still which takes `QualTypes` that are fixed point or integer
types and gets the `FixedPointSemantics` from them.
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.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3208
+ if (LHSWidth < CommonWidth)
+ LHS = LHSSign ? Builder.CreateSExt(LHS, CommonTy)
+ : Builder.CreateZExt(LHS, CommonTy);
----------------
bjope wrote:
> `LHS = Builder.CreateIntCast(LHS, CommonTy, LHSSign);`
>
> (I think that it even is possible to remove the condition and always do this
> (the cast will be a no-op if LHSWidth==CommonWidth))
Replaced this part with converting the operands to common semantics, then
regular addition.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3211
+ if (RHSWidth < CommonWidth)
+ RHS = RHSSign ? Builder.CreateSExt(RHS, CommonTy)
+ : Builder.CreateZExt(RHS, CommonTy);
----------------
bjope wrote:
> Same as above, this can be simplified as:
> RHS = Builder.CreateIntCast(RHS, CommonTy, RHSSign);
>
Same as above.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3214
+
+ // Align scales
+ unsigned CommonScale = std::max({LHSScale, RHSScale, ResultScale});
----------------
bjope wrote:
> Arithmetically I think that it would be enough to align the scale to
> ```
> std::max(std::min(LHSScale, RHSScale), ResultScale)
> ```
> As adding low fractional bits outside the result precision, when we know that
> those bits are zero in either of the inputs, won't impact any more
> significant bits in the result.
>
> Maybe your solution is easier and good enough for a first implementation. And
> maybe `opt` is smart enough to optimize this anyway. Or maybe codegen could
> be worse due to not using legal types. Or maybe codegen could be improved due
> to using sadd.sat in more cases?
> Lots of "maybes", so I don't think you need to do anything about this right
> now. It is just an idea from my side.
Same as above.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3223-3247
+ if (ResultWidth < CommonWidth) {
+ // In the event we extended the sign of both operands, the intrinsic will
+ // not saturate to the initial bit width of the result type. In this
case,
+ // we can default to use min/max clamping. This can arise from adding an
+ // operand of an int type whose width is larger than the width of the
+ // other fixed point operand.
+ // If we end up implementing the intrinsics for saturating ints to
----------------
leonardchan wrote:
> There is a case when we cannot use the sadd/uadd intrinsics because those
> intrinsics saturate to the width of the operands when instead we want to
> saturate to the width of the resulting fixed point type. The problem is that
> if we are given operands with different scales that require extending then
> shifting before adding, the bit width no longer matches that of the resulting
> saturated type, so the sat intrinsics instead saturate to the extended width
> instead of the result type width. This could be a problem with my logic
> though and there could be a better way to implement addition.
>
> Otherwise, as far as I can tell, this could be addressed by replacing the
> clamping with an intrinsic, which would create a use case for the
> signed/unsigned saturation intrinsics. Alternatively, the addition intrinsics
> could be expanded to also provide another argument that contains a desired
> saturation bit width, which would allow for collapsing the saturation type
> case into a call to this one intrinsic function.
>
Fixed by converting operands to common fixed point semantics, adding, then
converting the result. This ends up producing the same instructions as the
original tests.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:1393
+/// rules in N1169 4.1.4.
+QualType Sema::FixedPointConversions(ExprResult &FixedExpr,
+ ExprResult &OtherExpr, bool IsCompAssign)
{
----------------
ebevhan wrote:
> I'm not sure I understand why this is in a separate function. What part of
> this doesn't work in UsualArithmeticConversions, in a
> 'handleFixedPointConversion' similar to the other cases?
I might've misinterpreted the standard in that operations between fixed point
and other fixed point or integers didn't fall under usual arithmetic
conversions, but I can see how it would be simpler to keep in in UAC since
almost all checks on binary operations call this.
Added a changed `FixedPointConversions` to `handleFixedPointConversion`.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:1416
+ // converted to its corresponding signed fixed-point type and the resulting
+ // type is the type of the converted operand.
+ if (OtherTy->isSignedFixedPointType() &&
----------------
ebevhan wrote:
> I feel like this logic can be folded into the rank handling. Does it work
> properly if you give signed types higher rank than unsigned ones?
I think this is necessary since it seems the purpose of this is to align the
scales between signed and unsigned types if they are different. For signed and
unsigned types specifically, the standard also says `unsigned fixed-point
operand is converted to its corresponding signed fixed-point type` which I
think means it's an actual conversion.
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
