Page MenuHomePhabricator

[Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

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



This diff includes changes for the remaining _Fract and _Sat fixed point types.

signed short _Fract s_short_fract;
signed _Fract s_fract;
signed long _Fract s_long_fract;
unsigned short _Fract u_short_fract;
unsigned _Fract u_fract;
unsigned long _Fract u_long_fract;

// Aliased fixed point types
short _Accum short_accum;
_Accum accum;
long _Accum long_accum;
short _Fract short_fract;
_Fract fract;
long _Fract long_fract;

// Saturated fixed point types
_Sat signed short _Accum sat_s_short_accum;
_Sat signed _Accum sat_s_accum;
_Sat signed long _Accum sat_s_long_accum;
_Sat unsigned short _Accum sat_u_short_accum;
_Sat unsigned _Accum sat_u_accum;
_Sat unsigned long _Accum sat_u_long_accum;
_Sat signed short _Fract sat_s_short_fract;
_Sat signed _Fract sat_s_fract;
_Sat signed long _Fract sat_s_long_fract;
_Sat unsigned short _Fract sat_u_short_fract;
_Sat unsigned _Fract sat_u_fract;
_Sat unsigned long _Fract sat_u_long_fract;

// Aliased saturated fixed point types
_Sat short _Accum sat_short_accum;
_Sat _Accum sat_accum;
_Sat long _Accum sat_long_accum;
_Sat short _Fract sat_short_fract;
_Sat _Fract sat_fract;
_Sat long _Fract sat_long_fract;

This diff only allows for declaration of these fixed point types. Assignment and other operations done on fixed point types according to will be added in future patches.

Diff Detail


Event Timeline

leonardchan created this revision.May 15 2018, 4:03 PM
leonardchan retitled this revision from Addition of the remaining fixed point types and their saturated equivalents to [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents.May 16 2018, 9:41 AM

Updated formatting

ebevhan added inline comments.
206 ↗(On Diff #147568)

This error should be reworded.

Perhaps '_Sat' specifier is only valid on '_Fract' or '_Accum', not '%0'.

1139 ↗(On Diff #147568)

This variable name should probably be a different case.

1430 ↗(On Diff #147568)

You should probably use the 'getCorrespondingSaturatingType' function instead of disambiguating the whole thing here again.

45 ↗(On Diff #147568)

What about typedefs with the _Sat specifier on them?

Ka-Ka added a subscriber: Ka-Ka.May 23 2018, 1:07 AM
leonardchan marked 4 inline comments as done.
leonardchan added inline comments.
1430 ↗(On Diff #147568)

Also used getCorrespondingSignedType() for the signage

Forgot to include tests for the _Fract and _Sat keywords in c++ usage

ebevhan added inline comments.May 31 2018, 12:59 AM
6551 ↗(On Diff #149169)

These should probably be in ASTContext directly.

83 ↗(On Diff #149169)

I don't think the saturating types need separate configurations. Embedded-C says "Each saturating fixed-point type has the same representation and the same rank as its corresponding primary fixed-point type."

3614 ↗(On Diff #149169)

Is there a use for the isSat parameter?

1123 ↗(On Diff #149169)

Handling this case here means that placing _Sat on something other than exactly a fixed-point type is a parsing error rather than a semantic error. How does this handle _Sat on sugared types? Should _Sat on things like typedefs work?

typedef _Fract myfract;
_Sat myfract F;

The primary issue (and this is one that we have encountered as well) is that you cannot have a true _Sat typedef since _Sat only exists as part of builtin types. You need to desugar/canonicalize the type and then do getCorrespondingSaturatingType (or have getCorrespondingSaturatingType look at the canonical type internally).

1135 ↗(On Diff #149169)

IsFixedPointType can be used here as well.

1165 ↗(On Diff #149169)


1410 ↗(On Diff #149169)

The logic is a bit reversed. The default should be to select the signed variant, and if the TSS is unsigned, then convert it to the unsigned variant.

1609 ↗(On Diff #149169)

Other qualifiers like const and volatile are handled down here. Should the _Sat application be performed somewhere here instead?

leonardchan marked 7 inline comments as done.
leonardchan added inline comments.
1123 ↗(On Diff #149169)

I think _Sat is analogous to _Complex where it only works with specific builtin types, albeit the only builtin type _Complex doesn't work with is _Bool.

Currently this example would throw the error '_Sat' specifier is only valid on '_Fract' or '_Accum', not 'type-name' which is similar to what _Complex does when paired with a typedef:

typedef double mydouble;
mydouble _Complex D;  // _Complex type-name' is invalid

I don't see this as a big problem right now, but am willing to come back to this in the future if it becomes more urgent. For now, I added a test that asserts this error is thrown.

1410 ↗(On Diff #149169)

Using getCorrespondingUnsignedType(). Also changed getCorrespondingUnsignedType() to accept fixed point types.

ebevhan added inline comments.Jun 1 2018, 12:45 AM
2882 ↗(On Diff #149360)

This probably belongs up near the other predicates.

Also I think it's more common to simply pass QualType instead of a const QualType&.

1123 ↗(On Diff #149169)

That sounds reasonable. I have no strong opinions on it and I don't think we use the functionality anyway.

1612 ↗(On Diff #149360)

The braces aren't needed.

leonardchan marked an inline comment as done.

@ebevhan Does everything look good in this patch? I'm almost done with the next one. Writing tests for it still.

leonardchan added subscribers: rjmccall, jfb.

Yes, it looks good to me.

Actually, wait! One last thing I missed.

670 ↗(On Diff #149533)

This should take a PrintingPolicy like the others.

ebevhan added inline comments.Jun 7 2018, 4:52 AM
1612 ↗(On Diff #149533)

Also, this does not seem to invalidate the declarator.

leonardchan added inline comments.Jun 7 2018, 8:01 AM
670 ↗(On Diff #149533)

I think the PrintingPolicy may not be necessary because it's only used for getting the name of the current TypeSpecType. More specifically, for just differentiating vetween "bool" and "_Bool" for TST_bool. It also seems that other setters that don't touch TypeSpecType use PrintingPolicy, like SetTypeSpecComplex or SetTypeSpecSign

1612 ↗(On Diff #149533)

How so? I have tests under test/Frontend/fixed_point_errors.c that check for an error thrown if _Sat is used with non-fixed point types.

ebevhan added inline comments.Jun 7 2018, 8:14 AM
670 ↗(On Diff #149533)

You are correct, the ones that don't need it don't take it. I'm just being selfish since I need it downstream to disambiguate __sat and _Sat.

It's fine the way it is.

1612 ↗(On Diff #149533)

Hm, that is true. We don't have it downstream either. I'm not sure what the purpose of doing it is, but other invalid specifiers do declarator.setInvalidType(true);

I guess it isn't needed, just curious as to why it's done for some other ones.

ebevhan accepted this revision.Jun 13 2018, 4:34 AM

LGTM, but I'm not a code owner so maybe someone else should chime in if they have any input.

This revision is now accepted and ready to land.Jun 13 2018, 4:34 AM
ebevhan added inline comments.Jun 13 2018, 4:36 AM
44 ↗(On Diff #149533)

Wait; should these dso_local be {{.*}}?

leonardchan marked an inline comment as done.
leonardchan added inline comments.
44 ↗(On Diff #149533)

They should, I forgot to get the latest diff which checks this since not all targets produce dso_local.

ebevhan accepted this revision.Jun 14 2018, 6:25 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.