Page MenuHomePhabricator

[Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types
Needs ReviewPublic

Authored by leonardchan on Jun 21 2018, 2:40 PM.

Details

Summary

Add support for casting between fixed point types and integer types, floating point types, or other fixed point types.

Tested casts include implicit casting during assignment, passing as a function argument, returning from a function, and an explicit C style cast.

This patch introduces 6 different cast types: fixed to int, int to fixed, fixed to float, float to fix, fixed to boolean, and fixed to fixed.

Also included are:

  • A small fix for a bug where we could parse 0k as a fixed point literal.
  • Limit the targets tested on to x86_64-linux when checking LLVM IR. This is done to avoid not capturing extra keywords we did not expect when running these tests on other platforms.

Diff Detail

Repository
rC Clang

Event Timeline

leonardchan edited the summary of this revision. (Show Details)Jun 21 2018, 2:44 PM
leonardchan added a subscriber: scanon.
  • Add test case for fix where 0.0r would resolve as -1 when creating a fixed point literal.
rjmccall added inline comments.Jun 24 2018, 10:35 PM
include/clang/AST/ASTContext.h
1954

Are these opaque bit-patterns? I think there should be a type which abstracts over constant fixed-point values, something APSInt-like that also carries the signedness and scale. For now that type can live in Clang; if LLVM wants to add intrinsic support for fixed-point, it'll be easy enough to move it there.

lib/CodeGen/CGExprScalar.cpp
768

Can you explain the padding thing? Why is padding uniformly present or absent on all unsigned fixed point types on a target, and never on signed types? Is this "low bits set" thing endian-specific?

1801

Please abstract functions for doing this kind of arithmetic instead of inlining them into scalar emission. Feel free to make a new CGFixedPoint.h header and corresponding implementation file.

Also, it looks like you're assuming that the output type is the same size as the input type. I don't think that's actually a language restriction, right?

Would it be possible to add some form of target hook (perhaps to CodeGenABIInfo, which is already accessed with getTargetHooks) for fixed-point operations (maybe even some conversions)? As I've mentioned earlier, we emit both IR and intrinsics for many of these operations to make instruction selection possible. I suspect that any target that wants to utilize fixed-point effectively will need this as well.


I'm contemplating whether the unsigned padding bit clear before operations is needed at all. If you have the outlook that producing a value with a bit in that position is technically overflow and therefore undefined behavior, it shouldn't matter that the bit is set or not before operations; we already have UB. If you want to be sure that things 'work anyway', then you're better off clearing after operations instead to keep things consistent between operations instead of just before them.

I suspect that some of our tests depend on unsigned behaving this way, so we would have to look at that in case we decide to drop the clear altogether...


I also have a bit more feedback on previous patches that I missed at the time, but it feels wrong to add that here. How do you want me to take it?

include/clang/AST/ASTContext.h
1954

All of the parameters that determine these values (min, max, one) need to be sourced from the target, though. It's not like APSInt is aware of the dimensions of integers on the targets. I think these should return APSInt and not APInt, though.

Creating another abstraction on top of APInt is probably still quite useful, especially for certain operations (multiplication, division, saturating conversion, saturating operations). Probably not too useful for this patch since it doesn't deal with constant evaluation.

include/clang/Lex/LiteralSupport.h
76

I realized there was something off with this as well after the patch landed, but I wasn't sure what it was.

Could this cause problems with user-defined literals as well?

lib/AST/ASTContext.cpp
10260

Can't this be Ty->isSignedFixedPointType() || SameFBits?

I suspect this function can be simplified to look like the 'Min' one.

10283

This will give a nonsensical result for unsigned fract with !SameFBits.

lib/AST/ExprConstant.cpp
9501

The shift here will not produce the correct rounding behavior for fixed-point to integer conversion. E-C says Conversions from a fixed-point to an integer type round toward zero. However, simply shifting will always round towards -inf.

If the fixed-point number is negative, you need to add lowBits(Scale) before performing the shift.

This operation here is also not the same as the FixedPointToBool emission in CodeGen.

lib/CodeGen/CGExprScalar.cpp
768

I'll give an example with a fract type. Let's say fract is 16 bits wide. That means the signed fract will have a scale of 15 (15 fractional bits) and one sign bit, for a value range of [-1.0, 1.0). The LSB is worth 2^-15 and the MSB is worth -2^0, similar to signed integers.

The unsigned fract cannot be negative, but must also have a value range of 1.0. This means that either:

  • The scale remains the same (15), and the MSB is a padding bit. This bit would normally be the 2^0 bit, but since the range of the number cannot exceed 1, this bit will always be 0. In this case, the LSB is worth 2^-15 (same as the signed) and the MSB is not worth anything.
  • The scale is that of signed fract + 1 (16). All bits of the number are value bits, and there is no padding. The LSB is worth 2^-16 and the MSB is worth 2^-1. There is no bit with a magnitude of 2^0.

These rules also apply to accum types, since the number of integral bits in the unsigned accum types may not exceed that of the signed ones. Therefore, we either have to choose between having the same number of fractional bits as the signed ones (leaving the MSB as 0/padding) or having one more fractional bit than the signed ones.

The lowBits is just there to construct a mask to block out the MSB. There should be no endian consideration.

1790

Is there a reason these are all implemented here and not in EmitScalarConversion?

1796

As I mentioned in the ExprConstant case above, the shifting here is not enough to guarantee the correct rounding.

Also, I'm not sure I'm comfortable with relying on EmitScalarConversion for parts of these conversions. If you know what needs to be emitted (addition if negative, right shift, then extension) simply do that explicitly instead of trusting that EmitScalarConversion will do the right thing for you.

For the record, we do the shifting before the extension.

1801

I think that EmitScalarConversion will do 'the right thing' since it's being passed an input type of width N and converting it to a type of width M, but it's doing so 'stupidly' and isn't really aware of that one of them is a fixed-point type.

I'd much rather see the conversions be completely separate from EmitScalarConversion, implemented with explicit IR operations. Or have them moved to EmitScalarConversion completely. We've done the latter.

1815

I'm not sure I understand what 'both sides' are here. You should not have to compare the casted source integer at all; it should be possible to determine saturation based solely on the original value.

I think there are three cases (disregarding sign, which I know plays a part but I'm unsure at first glance how to handle):

  • src has fewer integral bits than dst has ibits; saturation cannot occur.
  • src has the same number of integral bits as dst has ibits; I believe it comes down to signedness here.
  • src has more integral bits than dst has ibits; this is where you need a comparison.

Ultimately it boils down to determining if the source integer is outside the value range of the integral portion of the fixed-point value, which should not require any casting of the integer other than to produce the value normally (as in Result above).

1882

Won't this be zero for unsigned fract if SameFBits is false?

1949

In our downstream, we interleave the rescaling and resizing differently. Grabbed from our consteval:

if (DestScale < SrcScale)
  Result = Result >> (SrcScale - DestScale);
Result = Result.extOrTrunc(DestWidth);
Result.setIsUnsigned(DestType->isUnsignedFixedPointType());
if (DestScale > SrcScale)
  Result = Result << (DestScale - SrcScale);

I'm not sure if this difference matters from a functional perspective at first glance. Something like:

if (DstScale < SrcScale)
  Result = isSigned ? Builder.CreateAShr(Result, SrcScale - DstScale)
                    : Builder.CreateLShr(Result, SrcScale - DstScale);

if (DstWidth != SrcWidth)
  // Resize
  Result = Builder.CreateIntCast(
      Result, CGF.CGM.getTypes().ConvertTypeForMem(DestTy), isSigned);

if (DstScale > SrcScale) {
  Result = Builder.CreateShl(Result, DstScale - SrcScale);
}

Also, the code here doesn't seem to handle saturation. That can get a bit tricky when casting between types of differing scale.

lib/Sema/SemaCast.cpp
2612

Why are these checks needed? Won't PrepareScalarCast guard against invalid fixed-point casts?

lib/Sema/SemaExpr.cpp
6904

I'm not sure why this is needed. All conversions between fixed-point types and other arithmetic types are defined, at least in Embedded-C (not in DSP-C, so we require a patch here).

Also, this is for the conditional operator, so getting an error about 'casts' seems like it could be very confusing. Is there anything that's preventing you from getting the err_typecheck_cond_incompatible_operands error at the very bottom?

7761

Same as the other case; why are these checks needed?

ebevhan added inline comments.Jun 26 2018, 1:49 AM
lib/AST/ExprConstant.cpp
9501

Oops, this has nothing at all to do with FixedPointToBool. Forget about that.

rjmccall added inline comments.Jun 26 2018, 1:07 PM
include/clang/AST/ASTContext.h
1954

Well, I think we can add the abstraction — in a separate patch that this can depend on — even if it doesn't actually provide operations that would be useful for constant-folding yet, and then we can stop introducing assumptions like this that constant values are passed around as APInts.

lib/CodeGen/CGExprScalar.cpp
768

I'll give an example with a fract type. Let's say fract is 16 bits wide. That means the signed fract will have a scale of 15 (15 fractional bits) and one sign bit, for a value range of [-1.0, 1.0). The LSB is worth 2^-15 and the MSB is worth -2^0, similar to signed integers.

Ah, I see, and now I've found where this is laid out in the spec, as well as where the spec leaves this representation question open for unsigned types.

Is this review debating whether or not to use a padded implementation? Do we not have any requirement to maintain compatibility with other compilers? Do other compilers differ on the representation, perhaps by target, or is there general agreement?

In the abstract, I think it would be better to fully use all the bits available, even if it means conversions are non-trivial.

1801

Yeah, I agree that using EmitScalarConversion as a subroutine here is really confusing, even if somehow it works.

ebevhan added inline comments.Jun 27 2018, 12:27 AM
include/clang/AST/ASTContext.h
1954

I suppose that's reasonable. I'm unsure of what operations exactly to expose (and how to expose them without confusing potential users) but that can probably be discussed in that patch.

lib/CodeGen/CGExprScalar.cpp
768

We had some discussions about representation in earlier patches. The target specification was a lot more free in the first revisions, but we locked it down to fewer parameters (scale options for accum types, fract scale being width-1, and the unsigned-signed scale flag) to make the implementation simpler.

I argued for adding the target option to set the unsigned scale to the same as the signed; Leonard's original design did not have it. The reason is simply that we use that representation downstream in our DSP-C implementation, and things would be harder to maintain for us if the upstream implementation did not support it. The only other Embedded-C implementation I know of is the one in avr-gcc, and I believe it uses all the bits for unsigned.

I've given some arguments for why our representation is better from an implementation perspective (conversion is easier, precision ranges are more uniform, hardware operations that operate on signed can be reused for unsigned) in previous patches, but I suspect that using all bits is a reasonable default.

rjmccall added inline comments.Jun 27 2018, 12:51 AM
lib/CodeGen/CGExprScalar.cpp
768

I'd be happy to read those discussions instead of asking you to summarize them if you wouldn't mind digging out a link.

That said, I agree that the right approach for Clang as a project is to allow the target to decide. If there were really an arbitrary number of implementations, that would be different, but from the spec it seems clear that there are exactly two reasonable choices.

ebevhan added inline comments.Jun 27 2018, 1:11 AM
lib/CodeGen/CGExprScalar.cpp
768

I think most of the discussion about representation is spread out in D46915, probably some here: