[Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals
ClosedPublic

Authored by leonardchan on May 15 2018, 4:22 PM.

Details

Summary

This diff includes the logic for setting the precision bits for each primary fixed point type in the target info and logic for initializing a fixed point literal.

Fixed point literals are declared using the suffixes

hr: short _Fract
uhr: unsigned short _Fract
r: _Fract
ur: unsigned _Fract
lr: long _Fract
ulr: unsigned long _Fract
hk: short _Accum
uhk: unsigned short _Accum
k: _Accum
uk: unsigned _Accum

Errors are also thrown for illegal literal values

unsigned short _Accum u_short_accum = 256.0uhk;   // expected-error{{the integral part of this literal is too large for this unsigned _Accum type}}

Diff Detail

Repository
rC Clang
There are a very large number of changes, so older changes are hidden. Show Older Changes
leonardchan retitled this revision from [Fixed Point Arithmetic] Set Fixed Point Precision Bits and Create Fixed Point Literals to [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals.Jun 6 2018, 8:11 PM
leonardchan edited the summary of this revision. (Show Details)
leonardchan added inline comments.
lib/AST/ExprConstant.cpp
9544

I believe clause 4.1.3 states that if FX_FRACT_OVERFLOW or FX_ACCUM_OVERFLOW are set to SAT then overflow behavior on the respective types is saturation. Otherwise, overflow has undefined behavior by default or if the pragma is set to DEFAULT.

lib/Sema/SemaExpr.cpp
3358

Replaced with getters for corresponding type followed by getters for information about the bits.

3434

Right, this is taken care of in the target creation.

ebevhan added inline comments.Jun 7 2018, 1:38 AM
include/clang/AST/Type.h
6552

This should probably take an APInt or APSInt.

Also, is there a reason to only have it produce unsigned numbers?

include/clang/Basic/LangOptions.def
308

Is it safe to have this as a flag? What about making it a target property?

include/clang/Basic/TargetInfo.h
89

I suspect it's still possible to calculate the ibits based on the fbits, even for _Accum.

Are the unsigned values needed? The fbits for unsigned _Fract are the same as for signed _Fract if SameFBits is set, and +1 otherwise. The same should go for unsigned _Accum, but I don't think it's entirely clear how this affects the integral part.

include/clang/Lex/LiteralSupport.h
116

This should return an APInt. That way you won't run the risk of losing any bits due to overflow or extraneous precision.

lib/AST/ExprConstant.cpp
9542

This doesn't saturate properly. Is that coming in a later patch?

9544

This would probably be simpler if the FixedPointValueToString function could produce signed values.

lib/AST/StmtPrinter.cpp
1540

Format this more like the one above to make fewer lines.

lib/AST/Type.cpp
3990

I think this would be better if it took a SmallString (or SmallVectorImpl) as reference and used that instead of returning a std::string.

Also should probably take an APInt/APSInt.

lib/Lex/LiteralSupport.cpp
1044

This should take an APInt (and use APInts for processing) to store the result in.

It should be possible to calculate exactly how many bits are needed to fit the literal before you start reading the digits. Overflow should not be a problem, but you might get precision loss in the fractional part. The calculation in our version is ceil(log2(10^(noof-digits))) + Scale but ours only handles normal decimal notation (123.456) so it might need to be extended to handle exponents and hex.

lib/Sema/SemaExpr.cpp
1247

I don't see how these semantics work properly. The specification requires that operations be done in the full precision of both types. You cannot convert the types before performing the operation like this, since the operation will not be done in full precision in that case.

The operator semantics of Embedded-C require the operand types of binary operators to be different. It's only when you've performed the operation that you are allowed to convert the result to the resulting type.

1295

I guess you haven't gotten there yet, but this should probably be before handleFloatConversion if you want to handle 'float + _fract' later.

3362

With APInts it should be possible to generalize this code to work with both accum and fract.

leonardchan marked 14 inline comments as done.
leonardchan added inline comments.
include/clang/AST/Type.h
6552

Done. It initially took only unsigned numbers since the literals could only be unsigned, but it makes sense to take signed also.

include/clang/Basic/LangOptions.def
308

Can do, but if the point of this option is just to set the Unsigned FBits to their corresponding Signed FBits, then would it be better instead to just ignore this flag altogether and have the target explicitly override the default Unsigned values with the corresponding Signed vals?

include/clang/Basic/TargetInfo.h
89

Similar to the previous comment, if we choose to make SameFBits dependent on/controlled by the target, then I think it would be better for the target to explicitly specify the integral bits since there could be some cases where there may be padding (such as for unsigned _Accum types if this flag is set), and in this case, I think it should be explicit that the bit_width != IBits + FBits.

We also can't fill in that padding for the unsigned _Accum types as an extra integral bit since the standard says that "each signed accum type has at least as many integral bits as its corresponding unsigned accum type".

For the unsigned _Fract values, I think we can get rid of them if we choose to keep the flag instead, but it would no longer apply to unsigned _Accum types since it would allow for extra padding in these types and would conflict with the logic of bit_width == IBits + FBits

For now, I actually think the simplest option is to keep these target properties, but have the target override them individually, with checks to make sure the values adhere to the standard.

lib/AST/ExprConstant.cpp
9542

Handling of saturated types will come in a later patch. For now this is focused on literals for which we cannot specify saturation.

lib/Lex/LiteralSupport.cpp
1044

I see. For the decimal exponent, the number of extra bits needed I think is ceil(Exponent * log2(10)) which requires parsing the exponent before parsing the integral or fractional part, assuming the exponent is positive.

For hex, the number of digits needed just for the integral and fractional part is just 4x the number of digits given in the hex literal + the exponent if it is positive.

With a bit more math we could also trim down the number of digits needed, but I think it's safe just to use the max number of digits needed.

lib/Sema/SemaExpr.cpp
1247

Initially the idea was to convert both sides to fixed point types, then perform standard binary operations between the fixed point types.

For the example, a fract * int would have the int converted to a fixed point type by left shifting it by the scale of the fract, multiplying, then right shifting by the scale again to get the resulting fract. The only unhandled thing is overflow, but the precision of the fract remains the same. The operands would also be casted up beforehand so there was enough space to store the result, which was casted down back to the original fract after performing the right shift by the scale.

Operations between fixed point types would follow a similar process of casting both operands to the higher rank fixed point type, and depending on the operation, more underlying shifting and casting would be done to retain full precision of the higher ranked type.

Though I will admit that I did not realize until now that multiplying a fixed point type by an integer does not require shifting the integer.

1295

A later patch adds a case in handleFloatConversion where we check if one of the operands is an integer and performs a cast from there.

ebevhan added inline comments.Jun 12 2018, 8:57 AM
include/clang/Basic/TargetInfo.h
89

Is it not the case that bitwidth != IBits + FBits for signed types only? Signed types require a sign bit (which is neither a fractional bit nor an integral bit, according to spec) but unsigned types do not, and if IBits and FBits for the signed and unsigned types are the same, the MSB is simply a padding bit, at least for _Accum (for _Fract everything above the fbits is padding).

My reasoning for keeping the number of configurable values down was to limit the number of twiddly knobs to make the implementation simpler. Granting a lot of freedom is nice, but I suspect that it will be quite hard to get the implementation working properly for every valid configuration. I also don't really see much of a reason for FBits != UFBits in general. I know the spec gives a recommendation to implement it that way, but I think the benefit of normalizing the signed and unsigned representations outweighs the lost bit in the unsigned type.

It's hard to say what the differences are beyond that since I'm not sure how you plan on treating padding bits. In our implementation, padding bits (the MSB in all of the unsigned types for us) after any operation are zeroed.

lib/Basic/TargetInfo.cpp
788

Shouldn't these be ShortAccumIBits >= UnsignedShortAccumIBits etc.?

lib/Lex/LiteralSupport.cpp
736

Is saw_fixed_point_suffix only used for this assertion? Doesn't isFract || isAccum imply saw_fixed_point_suffix?

1048

The function does a lot of overflow checks that I think are superfluous if the required bits are calculated properly. The only overflow check that should be needed is the one at the very bottom that truncates the APInt to the final size.

1064

This whole block is very diligent but I wonder how common overflow in the exponent is.

I'm unsure if LLVM has a helper function with atoll-like functionality, but if it does it's a lot less code to just use that instead. There's also APInt's fromString, but that seems to assert if the integer doesn't fit in the bitwidth. The required bitwidth can be calculated here, though.

1084

These two calculations look very similar to me. The only difference seems to be that the exponent is multiplied by 4 in the decimal case, and not in the hexadecimal case.

1140

I don't think breaking is dependent on this, it's dependent on whether or not the input is a hex digit.

We've already parsed the exponent further up, so we should be able to have an ExponentBegin that we can use as the loop bound.

(It's the same as SuffixBegin if there is no exponent)

1142

Does FoundExponent matter?

1174

This is technically Exponent.

1183

If you want to verify overflow here, you can check that Val.countLeadingZeroes() >= Scale.

1186

I don't think loops are needed here. BaseShift is essentially Exponent so it should be possible to implement as a pow(radix, abs(BaseShift)) and then either a mul or a div based on whether or not it was positive or negative.

1198

I think the final overflow check is the only one you need.

lib/Sema/SemaExpr.cpp
1247

I see how you've reasoned; this is how C normally works. The fract is of higher rank than int and therefore is the 'common type' of the operation. However, even though it is higher rank there is no guarantee that you can perform the operation without overflowing. And overflow matters here; the spec says that it must be done in the full precision (integral + fractional) of both types.

The only unhandled thing is overflow, but the precision of the fract remains the same. The operands would also be casted up beforehand so there was enough space to store the result, which was casted down back to the original fract after performing the right shift by the scale.

The precision remains the same (and while it doesn't have to be the same to perform an operation, it makes the implementation more regular; things like addition and subtraction 'just work'), but you cannot perform a conversion to fract *before* the operation itself, since if you do, there's nothing to 'cast up'. Casting up is needed for things like fract * fract to prevent overflow, but for fract * int you need to cast to a type that can fit both all values of the int and all values of the fract, and *then* you can cast up before doing the multiplication.

Operations between fixed point types would follow a similar process of casting both operands to the higher rank fixed point type, and depending on the operation, more underlying shifting and casting would be done to retain full precision of the higher ranked type.

This might work, but I feel there could be edge cases. The E-C fixed-point ranks are very odd as they don't reflect reality; short _Accum cannot be considered strictly 'above' long _Fract, but the former has a higher rank than the latter. Depending on how the types are specified for a target, implicit casts between fixed-point types might inadvertantly discard bits, even though the spec says that operations must be done in full precision.

1295

one of the operands is an integer

I assume you meant fixed-point here.

leonardchan marked 9 inline comments as done.
leonardchan added inline comments.Jun 12 2018, 2:08 PM
include/clang/Basic/TargetInfo.h
89

I see. The implementation would be much simpler this way. The initial idea was to treat the padding bits as "don't care" bits where we would mask only the data bits when doing operations like comparisons that look at the whole integer.

lib/Lex/LiteralSupport.cpp
736

saw_fixed_point_suffix is also used to check isFixedPointLiteral() which in turn is also used to check if not isIntegerLiteral() and not isFloatingLiteral() which are called early on in the constructor.

I could substitute where saw_fixed_point_suffix is set true to either setting isAccum or isFract but thought saving those for iterating through the suffixes would make checking for repeated r or k simpler.

1064

Used an APInt instead. It probably won't occur very often, but I would like to cover extreme cases.

1174

For hex types, the exponent base is 2 but every digit past the radix point implies division by 16, hence using a separate variable and multiplying by 4 afterwards.

1186

There's a chance though that the value represented by pow(radix, abs(BaseShift)) may be too large to fit into an integer which is why used the loop.

1198

I kinda want to keep the exponent check to cover extreme cases, but if it seems like too much I don't mind removing it.

lib/Sema/SemaExpr.cpp
1247

I see, so just to confirm, something like a fract * int would not result in any implicit casting between either operand, but any special arithmetic, like intermediate storage types or saturation handling, would be handled by the underlying IR?

So should really no conversions/implicit type casting should be performed here and instead all handling of arithmetic operations should happen somewhere during the codegen stage?

1295

Yup, my bad

ebevhan added inline comments.Jun 13 2018, 1:04 AM
lib/Lex/LiteralSupport.cpp
1186

True, but I suspect that can be solved by making BaseShift an APInt with the same bitwidth as Val.

lib/Sema/SemaExpr.cpp
1247

I see, so just to confirm, something like a fract * int would not result in any implicit casting between either operand, but any special arithmetic, like intermediate storage types or saturation handling, would be handled by the underlying IR?

Yes, for operations which require precision that cannot be provided by any of the existing types, there must be an 'invisible' implicit conversion to a type which can represent all of the values of either operand. This conversion cannot be represented in the AST as it is today.

The simplest solution is indeed to not have any implicit cast at all in the AST and resolve these conversions when needed (CodeGen and consteval are the locations I can think of), but ultimately it feels a bit dirty... I think that the best solution AST-wise is to define a completely new type class (perhaps FullPrecisionFixedPointType) that represents a fixed-point type with arbitrary width, scale, signedness and saturation. Then you can define ImplicitCasts to an instance of this type that can fit both the int and the fract. I don't know if adding this is acceptable upstream, though.

I think all of these rules must apply to fixed-fixed operations as well; a short accum * long fract must be done as a type that does not exist, similar to fixed-int. It's not clear how saturation should work here either...

I also noticed now that the spec says in regards to comparison operators, When comparing fixed-point values with fixed-point values or integer values, the values are compared directly; the values of the operands are not converted before the comparison is made. I'm not sure what this means.

3362

Does this (and the GetFixedPointValue with it) handle the case where fbits = scale - 1 for a fract?

ebevhan added inline comments.Jun 13 2018, 1:25 AM
include/clang/Basic/TargetInfo.h
89

That does work, but for binary operations it means you might need multiple maskings per operation. If you mask after every operation instead, you only need one. In our implementation, the only padding bit we have is the MSB in the unsigned types. We simply insert a 'bit clear' after every operation that produces an unsigned type (with some exceptions).

The E-C spec says that padding bits are 'unspecified', but implementation-wise, they have to be defined to something in order to operate on them. So long as you ensure that it is never possible to observe anything but zero in those bits from a program perspective, most operations just work. Naturally, there are ways to subvert this design. If you have a signed _Fract * and cast it to an unsigned _Fract * and the values happen to be negative, the padding bit will be set and you'll get exciting behavior.

Although... I just realized that the topmost half of the _Fracts in your configuration is all padding. That might make things a bit more complicated for things like signed types... Ideally the sign bit would be extended into the padding for those.

ebevhan added inline comments.Jun 13 2018, 8:20 AM
lib/AST/ExprConstant.cpp
9552

This looks very suspect to me as well... This might not respect padding on types whose sign bit is not the MSB, such as _Fract. The same goes for the overflow check above.

I think it's quite important that we define the semantics of what padding bits should contain. Probably zeroes for unsigned types and a sexted sign bit for signed types.

leonardchan added inline comments.Jun 13 2018, 10:54 AM
include/clang/Basic/TargetInfo.h
89

Yeah, I initially had 2 general purpose ideas for handling the padding:

  • Zero/sign extend them after all operations that modify the value (+, *, <<, casting to another type, ...). This way we know the sign part of a fixed point type, regardless of how much padding there is would, always extend to the padding and sign bit of the underlying scaled integer.
  • Masking only the data and sign bits before all operations that read the value and don't modify it (==, !=, <, ...). This way we only compare the non-padding bits whenever performing operations that require reading all bits in the underlying integer.

I wanted to choose the method that addressed operations fixed point types would least likely be used for, that is would these types be generally used more for arithmetic or comparisons. I chose the second method, but don't have any strong evidence to support this. This is also what the later patches implement but can be reworked.

lib/AST/ExprConstant.cpp
9552

Initially I had the _Accum and their corresponding _Fract types the same widths to avoid casting between _Accums to a different sized _Fract (ie short _Fract to short _Accum), but after thinking about it, it might be simpler in the end to just have the _Fract widths have half as much as the _Accum width and have the fractional bits be one less the width (unless SameFBits is set in which case there will still be only 1 padding).

This would also reduce the number of knobs that would be tweaked for different targets and help cover this case by effectively ignoring the padding bits.

ebevhan added inline comments.Jun 14 2018, 6:18 AM
include/clang/Basic/TargetInfo.h
89

I see what you mean... I also think it's more likely that the types will be used with arithmetic operators than comparisons. However, I don't have any evidence for it but it feels like it could be dangerous to let 'invalid' values hang around; it's safer to ensure that all values are correct after every operation. In our implementation we do the first method for our unsigned types. I wrote a small example (a FIR filter) to see what the overhead of using unsigned instead of signed was, and the x86 asm went from 20 to 23 instructions, which isn't huge. Granted, we do not use unsigned fixed-point types much (if at all) so overhead there isn't really a problem for us.

I think that one of the important benefits of the first method is that if unsigned fixed-point types with padding always have their padding cleared and the fractional bits are the same, then hardware operations for signed fixed-point types can be reused for the unsigned types.

I'm trying to come up with an example where things can go wrong if padding is not corrected after operations, but I can't seem to do it without invoking overflow, which is UB on both signed and unsigned fixed-point if I'm not mistaken. The E-C spec does not seem to mention anything explicit about what happens when you convert a negative signed fixed-point type to an unsigned one, so I think that's UB as well.

lib/AST/ExprConstant.cpp
9552

Yes, that sounds like a good idea. It is also the case for us that the scale of our _Fract types is the width - 1. This makes many operations that care about sign bits trivial, and conversion isn't that bad, it's just a sign/zero extension which should be no problem so long as the type widths are sensible (_Fract to _Accum would be 8->16, 16->32, 32->64 for the respective sizes).

I guess this means dropping the FractFBits configuration values? I suppose the AccumFBits values cannot be dropped as the specification does not have any restrictions between _Accum and _Fract (I think?).

ebevhan added inline comments.Jun 14 2018, 9:37 AM
lib/Sema/SemaExpr.cpp
1247

In any case, to clarify, I think there are two paths to consider. Either:

  • Add a new type class to the type system that encapsulates an arbitrary-precision fixed-point type that can be used for implicit casts when operating on fixed-point and integer types. This is in my opinion the cleaner solution, since it retains invariants on the types of operators and simplifies any logic that deals with operators; or,
  • Leave the operands of these operations uncasted. This is in some way simpler, since it doesn't require adding a whole new type, but it complicates other parts of the code. Anything that wants to deal with fixed-point operators will need to know how to do fixed-point conversion as well, which isn't a very good separation of responsibility IMO. It also breaks the C invariant of operands of arithmetic types being in a common type, which might be surprising to people.
leonardchan added inline comments.Jun 14 2018, 2:30 PM
include/clang/Basic/TargetInfo.h
89

I may go with your recommendation then since this is what you use and tested and I don't have any strong opinions. Also I think conversion from the negative signed to unsigned also falls under UB unless FX_FRACT/ACCUM_OVERFLOW is specified to SAT.

lib/AST/ExprConstant.cpp
9552

They do not have restrictions concerning fractional bits between _Accum and _Fract , but it is labelled as a recommendation to keep them consistent:

- Each unsigned accum type has the same number of fractional bits as its corresponding unsigned fract type.
- Each signed accum type has the same number of fractional bits as either its corresponding signed fract type or its corresponding unsigned fract type.
lib/Sema/SemaExpr.cpp
1247

I'm actually more of a fan for the second case. Aside, aside from the literal parsing in NumericLieralParser, wouldn't the only other place that would actually need to know about fixed point conversion be ScalarExprEmitter under CodeGen/CGExprScalar.cpp?

It seems that it's this class that creates the binary operations and other code gen classes like CodeGenFunction just make underlying calls to ScalarExprEmitter, so the actual conversion logic may just be contained here. Most of the implicit casting handled under UsualArithmeticConversions seems to be handled by VisitCastExpr under ScalarExprEmitter also, so adding another casting type would in the end just result in another case in the switch statement there, which in turn may just result in another call to ScalarExprEmitter.

I can see how it might be weird at first that these types don't fall under usual arithmetic, but the standard does specify that it wouldn't.

1247

Regarding comparison operators, my guess is that it means during comparison operations specifically, the actual underlying values of each operand are compared instead of having the special type conversions take place. That is, 1.0k != 1 but 1.0k == 128 (assuming scale of 7). If this is the case, we could actually save a few operations not having to do a shift on the integer.

I also can't seem to find a test case used by GCC where they explicitly compare a fixed point type against an integer. Normally, they instead assign the FP literal to an integral type, then compare that against another integer.

I'm referring to CONV_ACCUM_INT in
https://github.com/gcc-mirror/gcc/blob/e11be3ea01eaf8acd8cd86d3f9c427621b64e6b4/gcc/testsuite/gcc.dg/fixed-point/convert.h

Removed FractFBits property. Fractional bits for _Fract types are now one less the _Fract width unless SameFBits is specified. In that case, the number of FBits in unsigned _Fract is the same as that of signed _Fract, leaving one padding bit that would've been the sign.

  • Removed the functions for performing casting between fixed point types to a common type when performing a binary operation since operations should be performed on the types as is. Also this patch is meant more for declaring fixed point literals. Binary operations will come in later patches, but it's good to catch these types of things early.
  • Removed fixed_point_validation.c since that was the test for the logic described above.
ebevhan added inline comments.Jun 18 2018, 8:36 AM
lib/Sema/SemaExpr.cpp
1247

I'm actually more of a fan for the second case. Aside, aside from the literal parsing in NumericLieralParser, wouldn't the only other place that would actually need to know about fixed point conversion be ScalarExprEmitter under CodeGen/CGExprScalar.cpp?

ExprConstant (consteval) would also have to know, since the input expressions would be these 'unbalanced' binary operations. I'm not sure why it would affect literal parsing, though?

Regarding VisitCastExpr; in the first case, I'm not talking about adding a new CastKind, I'm talking about adding a whole new type altogether. This type would be just as much a fixed-point type as the builtin ones, just with a configurable width and scale. Then, something like this:

int * fract

where int is 32 bits and fract is 16 bits Q15, would become

(fract)((FullPrecFixedPoint<32+16, 0+15>)int * (FullPrecFixedPoint<32+16, 0+15>)fract)

The cast on the int is a CK_IntegerToFixedPointCast, and the cast on the fract is a CK_FixedPointCast. All values and operations are self-consistent and fully representable in the AST. Converting to and from a FullPrecFixedPoint type is no different from converting to and from, say, fract. They are both fixed-point types with width and scale; one is just built-in and the other is 'artificial'. The multiplication is performed like any other fixed-point operation, just in a higher width and (possibly higher) scale than either of the operands.

The issue I have with the second case is that the AST is somehow left 'unfinished'. There *are* casts there, but they are just not representable in the AST. In order to represent them, you would need to add these arbitrary-precision types.

1247

Regarding comparison operators, my guess is that it means during comparison operations specifically, the actual underlying values of each operand are compared instead of having the special type conversions take place. That is, 1.0k != 1 but 1.0k == 128 (assuming scale of 7). If this is the case, we could actually save a few operations not having to do a shift on the integer.

Right... That seems incredibly dangerous to me; I really hope this isn't what the spec means. 1.0 is by no means the same thing as 128. On top of that, it means that comparisons between fixed-point and integer can vary depending on the scale of the fixed-point type; this feels really shaky to me. Heck, if SameFBits is false, for a scale of 7, 0.5r == 64, but 0.5ur != 64. It might even be the case that 0.5r != 0.5ur. Absolutely bizarre, and incredibly confusing for programmers!

It might have been done this way to make it easy to inspect the raw bits of a fixed-point number, but why not just do a bit-preserving conversion and compare as an integer in that case?

DSP-C simply prohibits 'ambiguous' type conversions such as these to prevent this confusion from happening.

  • Removed CK_IntegralToFixedPoint to be added to later patch addressing conversion
  • Create CXString for FixedPointLiteral
leonardchan added inline comments.Jun 18 2018, 10:26 AM
lib/Sema/SemaExpr.cpp
1247

I also noticed that the doc provides a bitsfx function for getting this underlying integer value representation from a fixed point type. Still going to leave the comparisons as what I initially interpreted them as by comparing them normally (1.0k == 1).

1247

Understandable. Initially I thought it was a new cast for converting each fixed point type to this intermediate type. I will add this in a new separate patch that addresses conversions involving fixed point types since I feel this one is already large enough and was meant to address just parsing fixed point literals.

Just a couple more comments and then I think it looks good.

We can discuss the conversion and comparison issues in later patches.

include/clang/AST/ASTContext.h
1951

These can probably take QualType directly.

include/clang/Basic/TargetInfo.h
99

Could these and their accessors be called 'Scale' like in ASTContext? Only a consistency nit.

lib/Sema/SemaExpr.cpp
1247

Alright. I still suspect that isn't what the standard intends. You save a few instructions, but the comparison doesn't make sense from a value perspective.

We have an externally written test suite which has a bunch of tests for both DSP-C and E-C, and the E-C tests don't seem to behave the way you've described. I've also tried to use the support in avr-gcc to determine how it works and it doesn't seem to do a bitwise comparison there either.

We can discuss this in later patches.

leonardchan marked 2 inline comments as done.
leonardchan marked an inline comment as done.
leonardchan added inline comments.
lib/Sema/SemaExpr.cpp
1247

Oh, to clarify, I mean that I am performing the shifting necessary when comparing a fixed point to int or another fixed point. We are still comparing the values and not the bits directly.

ebevhan accepted this revision.Jun 19 2018, 8:44 AM
ebevhan added inline comments.
lib/Sema/SemaExpr.cpp
1247

Ah, okay, I misunderstood. I guess it'll be clearer when we get to those patches.

It might be that the spec means that the values should be compared in the full precision of both operands, just like with arithmetic operators. That does require some kind of conversion, though.

This revision is now accepted and ready to land.Jun 19 2018, 8:44 AM
This revision was automatically updated to reflect the committed changes.
leonardchan marked an inline comment as done.
rjmccall added inline comments.
include/clang/Basic/TargetInfo.h
90

Sorry for the extremely late review, but this really needs to be renamed. Please remember that other compiler maintainers are not experts in the feature that you happen to be currently implementing; they should be able to look at a variable name and understand what it means, and honestly "fbits" does not mean anything. UnsignedFixedPointHasNoPadding, maybe? Or you could shorten it by switching the polarity.

include/clang/Driver/Options.td
899

Okay, please also remember that *users* are not deeply embedded into the lore and wisdom of your implementation group. :) This is not an acceptable user-facing option name; please rename it to something with "fixed-point" in the name.

Also, if this is just for testing (it seems a bit early to decide that users will need an ability to override their target about this?), maybe it should just be a -cc1 option.

leonardchan added inline comments.Jun 28 2018, 8:50 AM
include/clang/Basic/TargetInfo.h
90

Made a new diff: https://reviews.llvm.org/D48727.

Opted for renaming to PaddingOnUnsignedFixedPoint.

include/clang/Driver/Options.td
899

Renamed to -fpadding-on-unsigned-fixed-point in https://reviews.llvm.org/D48727

See this bug here: https://bugs.llvm.org/show_bug.cgi?id=38161

This patch doesn't seem to properly consider integers, and ignores octals, which I suspect shouldn't be the case.

See this bug here: https://bugs.llvm.org/show_bug.cgi?id=38161

This patch doesn't seem to properly consider integers, and ignores octals, which I suspect shouldn't be the case.

I just requested an account on bugs.llvm.org and will respond here for now until I can comment on the thread there.

So the syntax for fixed point literals should be the same as that for floating point constants according to clause 6.4.4.2a in the embedded C spec (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1169.pdf). Integers and octal literals should not be supported since floating point literals can only be declared in decimal or hexadecimal notation.

I can see how the error messages don't seem clear and portray this, and should eventually change it so if the user does provide an integer/octal literal an error along the lines of "fixed point literals cannot be declared as integers" should be thrown instead.

The assertion is also something that I will address since this should not be thrown.

To sidetrack a bit also, we are limiting fixed point types to just C for now (https://reviews.llvm.org/D46084) so using auto to complete the type is not supported. This is until we come up with an itanium mangling for fixed point types (https://github.com/itanium-cxx-abi/cxx-abi/issues/56).

See this bug here: https://bugs.llvm.org/show_bug.cgi?id=38161

This patch doesn't seem to properly consider integers, and ignores octals, which I suspect shouldn't be the case.

I just requested an account on bugs.llvm.org and will respond here for now until I can comment on the thread there.

So the syntax for fixed point literals should be the same as that for floating point constants according to clause 6.4.4.2a in the embedded C spec (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1169.pdf). Integers and octal literals should not be supported since floating point literals can only be declared in decimal or hexadecimal notation.

Then it seems you've missed that part... These literals are allowing integers without issue at the moment.

I can see how the error messages don't seem clear and portray this, and should eventually change it so if the user does provide an integer/octal literal an error along the lines of "fixed point literals cannot be declared as integers" should be thrown instead.

Seems like that could work.

The assertion is also something that I will address since this should not be thrown.

To sidetrack a bit also, we are limiting fixed point types to just C for now (https://reviews.llvm.org/D46084) so using auto to complete the type is not supported. This is until we come up with an itanium mangling for fixed point types (https://github.com/itanium-cxx-abi/cxx-abi/issues/56).

At the moment you are NOT limiting to C. You'll have to add quite a view checks at the moment in order to make that limitation. That said `auto' was for convienience, it had nothign to do with the bug itself.

@erichkeane I created a patch at https://reviews.llvm.org/D49327 with the fix for this and added you as a reviewer.