This is an archive of the discontinued LLVM Phabricator instance.

[Fixed Point Arithmetic] Fixed Point and Integer Conversions
ClosedPublic

Authored by leonardchan on Jan 17 2019, 8:35 PM.

Details

Summary

This patch includes the necessary code for converting between a fixed point type and integer. This also includes constant expression evaluation for conversions with these types.

Diff Detail

Repository
rC Clang

Event Timeline

rjmccall added inline comments.Jan 17 2019, 10:33 PM
clang/lib/AST/ExprConstant.cpp
9839 ↗(On Diff #182456)

Can most of this reasonably be a method on APFixedPoint? (And likewise for CK_IntegralToFixedPoint.) If nothing else, I suspect you are going to want these when it comes to things like the compound assignment operators, which do implicit conversions internally that aren't directly expressed in the AST.

ebevhan added inline comments.Jan 18 2019, 1:31 AM
clang/lib/AST/ExprConstant.cpp
9825 ↗(On Diff #182456)

The Result < 0 is more clearly expressed as Result.isNegative() I think.

clang/lib/CodeGen/CGExprScalar.cpp
1499 ↗(On Diff #182456)

I think this block can be simplified significantly by simply rounding up before conversion. So, something like:

%lt = icmp slt %val, 0
%rounded = add %val, lowBits(Scale)
%sel = select %lt, %rounded, %val
... rescale/resize ...
clang/lib/Sema/SemaChecking.cpp
11096 ↗(On Diff #182456)

Seems like a lot of logic in these blocks is duplicated from the code in ExprConstant.

leonardchan marked 6 inline comments as done.
leonardchan added inline comments.
clang/lib/AST/ExprConstant.cpp
9825 ↗(On Diff #182456)

Ah, so I ran into something similar with the patch preceding this in APFixedPoint::convert(). isNegative() is a method of APInt which doesn't care about signage. It just checks if the last bit is set. Result < 0 calls APSInt::operator<() which cares about the sign and checks if this signed int is less than zero.

have no big problem with this, but if Result.isNegative() is more readable, I could also add Result.isSigned() which together effectively does the same thing as Result < 0.

9839 ↗(On Diff #182456)

Compressed them into APFixedPoint::convertToInt() and APFixedPoint::getFromIntValue()

clang/lib/Sema/SemaChecking.cpp
11096 ↗(On Diff #182456)

Yeah. I moved into APFixedPoint::getFromIntValue() to simplify this.

ebevhan added inline comments.Jan 21 2019, 7:26 AM
clang/lib/AST/ExprConstant.cpp
9825 ↗(On Diff #182456)

This makes sense, but you're already checking if the value is signed in the line above, so it shouldn't be an issue.

clang/test/Frontend/fixed_point_conversions.c
426 ↗(On Diff #182717)

There are no tests here for what you get if you convert an integer to a fixed-point type with a larger integral part than the integer has.

437 ↗(On Diff #182717)

Conversions like this are a bit odd. There shouldn't be a need to upsize/upscale the container before the saturation, should there?

leonardchan marked 6 inline comments as done.

Oh I forgot to submit the comments.

clang/lib/AST/ExprConstant.cpp
9825 ↗(On Diff #182456)

Ah, forgot about that.

clang/test/Frontend/fixed_point_conversions.c
426 ↗(On Diff #182717)

Added to the end of TestIntToFixedPoint

437 ↗(On Diff #182717)

I see. You're saying that we can just check directly if the value exceeds 255 (or goes under -256) since the range of target values can always fit in the range of source values. Therefore we do not need to cast up since the only reason we would need to is if converting to a type with a greater source range.

In this case, we could technically have a special case for integers where I think we can perform the saturation checks without the initial sign extension. I think it might be simpler though if in EmitFixedPointConversion, we treat an integer as a 'zero-scale fixed point number'. I think the reason the upsizing occurs in the first place is so that we satisfy the first FX conversion rule of calculating the result with full precision of both operands.

ebevhan added inline comments.Jan 25 2019, 12:44 AM
clang/test/Frontend/fixed_point_conversions.c
437 ↗(On Diff #182717)

I see. You're saying that we can just check directly if the value exceeds 255 (or goes under -256) since the range of target values can always fit in the range of source values. Therefore we do not need to cast up since the only reason we would need to is if converting to a type with a greater source range.

That's right. Though, for integer to fixed-point conversion, I don't think you ever need to upcast first; either the integer is larger than or equal to the integral part of the fixed-point type, in which case we can check the magnitude in the type as is, or it's smaller, and we don't have to do any saturation.

I think it might be simpler though if in EmitFixedPointConversion, we treat an integer as a 'zero-scale fixed point number'.

Isn't this already the case? The semantics for an integer type are fetched as a zero scale fixed-point type and used that way (except when the target is an integer due to the rounding requirement).

leonardchan marked an inline comment as done.Jan 25 2019, 2:34 PM
leonardchan added inline comments.
clang/test/Frontend/fixed_point_conversions.c
437 ↗(On Diff #182717)

That's right. Though, for integer to fixed-point conversion, I don't think you ever need to upcast first; either the integer is larger than or equal to the integral part of the fixed-point type, in which case we can check the magnitude in the type as is, or it's smaller, and we don't have to do any saturation.

I see. I think this would be more of a matter then of when checking for saturation, we either should check against the source value after resizing and scaling (container), or the source by itself before resizing and scaling. Actually, I think that when comparing saturation against the source value, we could save an instruction since we can just generate a constant to compare the source against instead of comparing against a potentially shifted value. I think this could work when converting from fixed point types as well.

With saturation conversion, (if the target scale >= src scale) currently we could generate up to 7 instructions:

  • 1 resize + 1 shift on the result if target scale > src scale
  • 1 cmp gt + 1 select for clamping to max
  • 1 cmp lt + 1 select for clamping to min
  • 1 resize if container width != target width

I think we don't need either the first or last resize if the constants that we check against are the respective max's/min's of the target type against the source. I think this deserves a patch on it's own though since it could change a bunch of tests that depend on EmitFixedPointConversion.

Isn't this already the case? The semantics for an integer type are fetched as a zero scale fixed-point type and used that way (except when the target is an integer due to the rounding requirement).

What I meant by this was that we are already doing the right thing in that we calculate the result with the full precision of both operands.

leonardchan marked an inline comment as done.
leonardchan added inline comments.
clang/test/Frontend/fixed_point_conversions.c
437 ↗(On Diff #182717)

Added this change in D57553 and made it a child of this patch.

ebevhan added inline comments.Feb 7 2019, 1:57 AM
clang/test/Frontend/fixed_point_conversions.c
437 ↗(On Diff #182717)

I think the patch demonstrated to me that this emission can't be optimized in general without breaking the minmax pattern on the saturation, and that is very useful in some cases.

What I think I'm concerned about is conversions like int -> _Sat _Fract, where there really is no point to upscaling at all, since the resulting will either be -1, 0 or ~0.999.

leonardchan marked an inline comment as done.
leonardchan added inline comments.Feb 20 2019, 5:53 PM
clang/test/Frontend/fixed_point_conversions.c
437 ↗(On Diff #182717)

I think upscaling is also necessary in this case if we decide to keep the minmax pattern for all conversion cases, otherwise we'd still be comparing against the result:

%0 = load i32, i32* %i, align 4
%1 = icmp sgt i32 %0, 1
%satmax = select i1 %1, i32 32767, i32 %0
%2 = icmp slt i32 %0, 1
%satmin = select i1 %2, i32 -32768, i32 %satmax
store i16 %resize1, i16* %sat_f, align 2

However, this takes only 6 instructions whereas the pattern we currently have takes 9:

%0 = load i32, i32* %i, align 4
%resize = sext i32 %0 to i47
%upscale = shl i47 %resize, 15
%1 = icmp sgt i47 %upscale, 32767
%satmax = select i1 %1, i47 32767, i47 %upscale
%2 = icmp slt i47 %satmax, -32768
%satmin = select i1 %2, i47 -32768, i47 %satmax
%resize1 = trunc i47 %satmin to i16
store i16 %resize1, i16* %sat_f, align 2

We could say that for specific cases, we compare against the source value and others we do minmax.

leonardchan marked an inline comment as done.Feb 20 2019, 5:54 PM
leonardchan added inline comments.
clang/test/Frontend/fixed_point_conversions.c
437 ↗(On Diff #182717)

Woops, the first example should have i16s in the select instructions, not i32s.

@rjmccall @ebevhan @bjope *ping* any other comments on this patch?

A couple tweaks, but otherwise LGTM.

clang/include/clang/AST/OperationKinds.def
203 ↗(On Diff #187706)

This is super-picky, but please either just say "integral" (without an article) or spell out "an integral type".

clang/include/clang/Basic/FixedPoint.h
157 ↗(On Diff #187706)

Please specify what happens to the overflow flag if there's a fractional component.

bjope added inline comments.Feb 27 2019, 1:15 AM
clang/lib/CodeGen/CGExprScalar.cpp
1223 ↗(On Diff #187706)

nit: May skip braces on this if

1230 ↗(On Diff #187706)

nit: may skip braces on this if.
nit: no need to use "else".

bjope added inline comments.Mar 1 2019, 12:19 AM
clang/test/Frontend/fixed_point_conversions.c
45 ↗(On Diff #187706)

Perhaps we should verify that we do not mess up signed values when using the -fpadding-on-unsigned-fixed-point option (having CHECK, DEFAULT and SAME as check prefixes)?

(nit: I also see that we now use different names for this check prefixes in different test files, which could be confusing. Why is it called SAME here?)

66 ↗(On Diff #187706)

A little bit confusing to be with this mix of having checks left-aligned (on separate lines) and some checks appended like this.
I can see that it already is like that before, so maybe it should be cleaned up in a separate commit (before this commit).

(I'm not so familiar with clang tests, so maybe it is common to have the FileCheck checks at the end of the line. But I've never seen that in llvm.)

leonardchan marked 11 inline comments as done.
leonardchan added inline comments.
clang/test/Frontend/fixed_point_conversions.c
45 ↗(On Diff #187706)

Perhaps we should verify that we do not mess up signed values when using the -fpadding-on-unsigned-fixed-point option (having CHECK, DEFAULT and SAME as check prefixes)?

Done

(nit: I also see that we now use different names for this check prefixes in different test files, which could be confusing. Why is it called SAME here?)

Sorry forgot to update these prefixes.

66 ↗(On Diff #187706)

Will change this, but would be easier for me if it was done after this commit.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 5 2019, 4:27 PM
This revision was automatically updated to reflect the committed changes.