Page MenuHomePhabricator

[Fixed Point Arithmetic] Addition of the Fixed Point _Accum type
ClosedPublic

Authored by leonardchan on Apr 25 2018, 2:31 PM.

Details

Summary

This diff includes changes for supporting the following types.

// Primary fixed point types
signed short _Accum s_short_accum;
signed _Accum s_accum;
signed long _Accum s_long_accum;
unsigned short _Accum u_short_accum;
unsigned _Accum u_accum;
unsigned long _Accum u_long_accum;

// Aliased fixed point types
short _Accum short_accum;
_Accum accum;
long _Accum long_accum;

This diff only allows for declaration of the fixed point types. Assignment and other operations done on fixed point types according to http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1169.pdf will be added in future patches. The saturated versions of these types and the equivalent _Fract types will also be added in future patches.

The tests included are for asserting that we can declare these types.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

LGTM with a couple additions.

lib/CodeGen/CodeGenTypes.cpp
448 ↗(On Diff #147560)

Add TODO for accum types using a different drawf type.

lib/Index/USRGeneration.cpp
731 ↗(On Diff #147560)

You can make the 'c' a Twine instead. That will let you inline these in their respective locations as c = "~UA" for instance.

leonardchan marked 2 inline comments as done.May 22 2018, 9:33 AM
leonardchan added inline comments.
lib/Index/USRGeneration.cpp
731 ↗(On Diff #147560)

So Twine also isn't assignable. If I still want to keep the pattern of assigning to a temporary variable, I could instead just make c a string.

leonardchan marked an inline comment as done.
jakehehrlich accepted this revision.May 22 2018, 3:59 PM
This revision is now accepted and ready to land.May 22 2018, 3:59 PM

pulled changes from source tree

rsmith added inline comments.
include/clang/Basic/DiagnosticCommonKinds.td
172 ↗(On Diff #148121)

Diagnostics should not be capitalized. Also, we generally allow conforming C extensions to be used in other languages unless there is a really good reason not to.

lib/AST/ASTContext.cpp
6261 ↗(On Diff #148121)

llvm_unreachable should only be used for cases where you believe the situation to be impossible. I would add these cases to the return ' '; case above (for Float16 / Float128 / Half) for now.

lib/AST/ExprConstant.cpp
7323–7325 ↗(On Diff #148121)

Have you checked what value GCC uses for these? Using integer_type_class for a non-integer value seems very strange to me, but then they probably are *passed* as integers, so maybe it's correct?

lib/AST/ItaniumMangle.cpp
2552 ↗(On Diff #148121)

Please check what GCC uses to mangle these, and follow suit; if GCC doesn't have a mangling, you can use a vendor mangling (u6_Accum) or produce an error for now, but please open an issue at https://github.com/itanium-cxx-abi/cxx-abi/ to pick a real mangling.

lib/CodeGen/CGDebugInfo.cpp
678–681 ↗(On Diff #148121)

@echristo @dblaikie Is this appropriate?

lib/CodeGen/CodeGenTypes.cpp
448 ↗(On Diff #147560)

I think this TODO was added to the wrong file. Should it be in CGDebugInfo?

lib/CodeGen/ItaniumCXXABI.cpp
2684 ↗(On Diff #148121)

Note this comment :)

lib/Index/USRGeneration.cpp
691 ↗(On Diff #145993)

@rjmccall Is this an acceptable USR encoding? (Is our USR encoding scheme documented anywhere?)

lib/Sema/SemaType.cpp
1390–1394 ↗(On Diff #148121)

As noted earlier, we should not have such a restriction.

1424 ↗(On Diff #148121)

No need for a diagnostic as this is already caught elsewhere.

CC: Argyrios for the USR question.

jfb added a subscriber: jfb.May 22 2018, 9:01 PM

This is on by default for any version of C? AFAICK _Accum isn't on the C17 draft that I have, I'd expect to have to specify a command-line flag pertaining to TR 18037 to get this. At a minimum I'd be OK having it with the GNU variant of C, but not the __ANSI_C__ one.

There are at least three good reasons to make sure this can enabled/disabled by a flag:

  • We have to anticipate that introducing new keywords will cause some compatibility problems. New language standards that introduce new keywords can be disabled using -std=<something earlier>. We shouldn't let this bypass that just because it's an out-of-band addition to the base language.
  • It seems likely to me that this feature will be target-restricted.
  • It seems plausible to me that development of this feature might continue past the Clang 7 branch date, and we shouldn't release a compiler with significantly-incomplete features on by default.

That said, when it's ready, I think it should probably be on by default on the targets that support it.

ebevhan added inline comments.
lib/AST/ASTContext.cpp
1775 ↗(On Diff #148148)

Please give the types their own width and alignment accessors/variables in TargetInfo and use those instead of reusing the existing ones.

lib/Sema/SemaType.cpp
1395 ↗(On Diff #148148)

This (and the rest of the fixed-point support) should be behind its own option. The error should reflect this as well.

Ka-Ka added a subscriber: Ka-Ka.May 23 2018, 1:00 AM
leonardchan added inline comments.
include/clang/Basic/DiagnosticCommonKinds.td
172 ↗(On Diff #148121)

We decided not to allow fixed point types in other languages because there is no specification provided in N1169 for addressing some features in other languages. Using C++ as an example, N1169 does not provide recommended characters when name mangling so we do not allow this in C++.

lib/AST/ItaniumMangle.cpp
2552 ↗(On Diff #148121)

It seems that GCC uses the characters for each fixed point type's corresponding integral type (https://github.com/gcc-mirror/gcc/blob/master/gcc/cp/mangle.c). Will follow up on this if we end up enabling fixed point types for C++.

lib/CodeGen/CGDebugInfo.cpp
678–681 ↗(On Diff #148121)

My bad, this should be changed to DW_ATE_signed_fixed and DW_ATE_unsigned_fixed

lib/Index/USRGeneration.cpp
691 ↗(On Diff #145993)

I chatted with @sammccall about this who said it was ok to add these types if no one opposed this. I posted this on cfe-dev also and it seemed that no one spoke up about it, so I thought this was ok.

I also couldn't find any standard or documentation about reserving characters for USR. It doesn't seem that USR is also parsed in any way, so I don't think I'm breaking anything (running ninja check-all passes).

And unless we also do enable this for C++, this code actually may not be run since this method will not be visited if limited to C.

leonardchan added inline comments.May 23 2018, 11:22 AM
include/clang/Basic/DiagnosticCommonKinds.td
172 ↗(On Diff #148121)

Actually, scratch that. We will be enabling it since GCC does. Will update this and other relevant C++ related code appropriately.

jfb added a comment.May 23 2018, 11:34 AM

Actually, scratch that. We will be enabling it since GCC does. Will update this and other relevant C++ related code appropriately.

Could you also add tests which mix _Accum with volatile, _Atomic, _Complex, constexpr, inline?

leonardchan added inline comments.May 23 2018, 5:58 PM
include/clang/Basic/DiagnosticCommonKinds.td
172 ↗(On Diff #148121)

Actually, the main thing that was preventing us from allowing this in C++ was no standardized characters for name mangling. GCC seems to use the same characters as some integral types (short _Accum uses s, _Accum uses i, ...) but this would mean that a function that takes a short _Accum as a sole argument would also be mangled the same as a similarly named function that takes a short.

Would copying GCC take priority over not having characters specific for these types? This standard also proposes 24 different types, of which only 6 are included in this patch.

After further discussion, we think the best approach for now would be only supporting fixed point types in C, then go back and support C++ once there is a standardized way for mangling the fixed point types under itanium.

rjmccall added inline comments.May 23 2018, 9:12 PM
include/clang/Basic/DiagnosticCommonKinds.td
172 ↗(On Diff #148121)

That makes it sound like GCC ignores _Accum and just mangles the unmodified type, which is clearly a bug that should not be imitated. You should raise this issue to the Itanium C++ ABI group and pick something unambiguous as a placeholder.

sammccall added inline comments.May 24 2018, 1:12 AM
lib/Index/USRGeneration.cpp
691 ↗(On Diff #145993)

If the conclusion is to only allow these types for C, then you could punt on this by setting IgnoreResult so USR generation fails. As you say USRs only include type information in C++.

I think nothing parses them except humans, and using a prefix character is probably better than eating 24 single characters. But one prefix character + one trailing character might be less surprising.

leonardchan updated this revision to Diff 148445.EditedMay 24 2018, 10:47 AM
  • Reverted changes involving name mangling since we will only support c++ for now. Will concurrently raise an issue on https://github.com/itanium-cxx-abi/cxx-abi/ to get characters for name mangling.
  • Added a flag that needs to be provided to enable usage of fixed point types. Not including this flag and using fixed point types throws an error. Currently, this patch allows for these types to be used in all versions of C, but this can be narrowed down to specific versions of C.
  • An error is thrown when using fixed point types in C++.
  • Fixed point types are ignored during USRGeneration since the type only gets mangled in C++.
  • Fixed point types get their own width and alignment accessors/variables in TargetInfo.
  • Updated debug info to use DW_ATE_signed_fixed and DW_ATE_unsigned_fixed.
  • Added tests mixing _Accum with other type specifiers
leonardchan marked 6 inline comments as done.May 24 2018, 10:53 AM
leonardchan added inline comments.
lib/CodeGen/ItaniumCXXABI.cpp
2684 ↗(On Diff #148121)

Returned false for these types now. Not sure if these types should also be added to EmitFundamentalRTTIDescriptors since the OCL types do not appear under there.

jfb added a comment.May 24 2018, 11:00 AM

Can you also add a test for _Bool _Accum.

Also, -enable-fixed-point -x c++ failing.

lib/AST/ExprConstant.cpp
7361 ↗(On Diff #148445)

"its"

lib/Basic/TargetInfo.cpp
45 ↗(On Diff #148445)

This seems weird because Targets which don't have these values for the non-Accum versions will have .e.g. sizeof(short) != sizeof(short _Accum). Is there a point in ever having _Accum differ in size, width, and alignment from the underlying type? If not, can you set these values after the sub-target has specified its preferences?

leonardchan updated this revision to Diff 148481.EditedMay 24 2018, 2:17 PM
leonardchan marked 2 inline comments as done.
  • Added test case for _Bool _Accum
  • Getters for the _Accum bit widths return values for their corresponding integral types (ie. sizeof(short _Accum) == sizeof(short)). The only case where this may not happen is if the target architecture uses 16 bits for an int. N1169 requires that a signed/unsigned _Accum hold at least 15 fractional bits and 4 integral bits. To be able to fit these bits, the size is upgraded to that of a long which is guaranteed to be large enough to hold them.
  • Added test for checking the bit widths.
In D46084#1111374, @jfb wrote:

Can you also add a test for _Bool _Accum.

Also, -enable-fixed-point -x c++ failing.

.
Done. Also the failing c++ case is under test/Frontend/fixed_point_errors.cpp

ebevhan added inline comments.May 24 2018, 2:57 PM
include/clang/Basic/TargetInfo.h
382 ↗(On Diff #148481)

I'm not sure I agree with this interpretation. It's simply not correct to consider 'short' the 'underlying type' of 'short _Accum', 'int' the 'underlying type' of '_Accum', etc. They are wholly independent and should have separate settings altogether.

Asserting/ensuring that a target has set an invalid width for its types should be done separately. This currently feels a bit like the TargetInfo is guessing.

(For the record, the reason I'm requesting this change is because this implementation does not work for us downstream.)

Re-added individual getters/members for _Accum types

include/clang/Basic/TargetInfo.h
382 ↗(On Diff #148481)

You're right. They should be different types. I was stuck in the mindset that short _Accum should be tied to short, _Accum be tied to int, etc.

With the exception of the two inline comments, this looks good to me now!

include/clang/Basic/LangOptions.def
301 ↗(On Diff #148506)

Just a minor nit... The 'Enabled' is superfluous.

include/clang/Driver/Options.td
882 ↗(On Diff #148506)

Shouldn't this be an -f flag, like -ffixed-point? I'm not for or against either, just wondering what anyone else thinks.

Changed flag names

leonardchan marked 2 inline comments as done.May 25 2018, 11:20 AM
leonardchan added inline comments.
include/clang/Driver/Options.td
882 ↗(On Diff #148506)

Makes sense since this flag is target independent.

leonardchan marked an inline comment as done.May 25 2018, 11:21 AM

Hi all, I think I've addressed all comments on this patch and will be committing tomorrow morning unless anyone has any more comments they'd like to bring up.

dstenb added a subscriber: dstenb.May 30 2018, 7:36 AM

Sorry for the late notice; I missed something.

include/clang/Basic/TokenKinds.def
393 ↗(On Diff #148637)

I believe that having KEYALL will enable the keyword even if -fno-fixed-point is given. Is this desired? It will mean that _Accum will not be a valid identifier in standard C regardless of the flag.

jfb added inline comments.May 30 2018, 10:41 AM
include/clang/Basic/TokenKinds.def
393 ↗(On Diff #148637)

That seems fine: identifiers starting with underscore and a capital letter already aren't valid identifiers in C and C++ because they're reserved for the implementation.

leonardchan added inline comments.May 30 2018, 10:57 AM
include/clang/Basic/TokenKinds.def
393 ↗(On Diff #148637)

I think my test for -fno-fixed-point already catches this, but I did not notice until now the KEYNOCXX keyword until now. Using this instead allows for not having to check if the language is c++ since _Accum is no longer treated as a typename. The corresponding test checking fixed points in c++ has been updated to reflect this.

rsmith accepted this revision.May 30 2018, 6:18 PM

Just minor comments. Feel free to commit after handling them.

include/clang/Basic/LangOptions.def
301 ↗(On Diff #148506)

There's still a superfluous "are enabled" in the description. Please consult the file comment:

// The Description field should be a noun phrase, for instance "frobbing all
// widgets" or "C's implicit blintz feature".

So just "fixed point types" would be appropriate. (And yes, many of the options in this file get this wrong.)

include/clang/Basic/TokenKinds.def
393 ↗(On Diff #148637)

Just to make sure we're on the same page: it's OK to disable this feature for C++ while you're working on it, but it needs to support C++ by the time you're done.

include/clang/Driver/Options.td
893 ↗(On Diff #149162)

Only the flag that enables a non-default mode should be a CC1Option.

include/clang/Serialization/ASTBitCodes.h
960 ↗(On Diff #149162)

Spurious comment, probably from a merge conflict?

lib/AST/ExprConstant.cpp
7361–7362 ↗(On Diff #149162)

GCC's implementation is irrelevant; please only talk about its observable behavior if you think this deserves a comment.

lib/AST/ItaniumMangle.cpp
2552 ↗(On Diff #148121)

OK for now, but as noted, this will need to be addressed eventually.

lib/Basic/TargetInfo.cpp
45 ↗(On Diff #148445)

I'm uncomfortable about opting all targets into this behavior with these default values; this will result in an ABI break if later a target updates these to the correct values. A per-target AccumSupported flag would help substantially. But this is OK for the short term while you're still working on the feature.

We'll also need the target to inform us of the number of integer and fractional bits for each such type.

lib/CodeGen/ItaniumCXXABI.cpp
2684 ↗(On Diff #148121)

OK, we'll need to remember to revisit this when adding C++ support.

lib/Sema/SemaType.cpp
1409 ↗(On Diff #149162)

No break after unreachable -- this is dead code.

1424 ↗(On Diff #149162)

Likewise here.

ebevhan added inline comments.May 31 2018, 12:21 AM
lib/Basic/TargetInfo.cpp
45 ↗(On Diff #148445)

We'll also need the target to inform us of the number of integer and fractional bits for each such type.

I believe the only one that is needed is for the number of fractional bits; the number of integer bits is implied by the difference between the type width and fractional bits. I think I mention this in one of the other patches.

leonardchan marked 6 inline comments as done.
leonardchan added inline comments.
include/clang/Basic/TokenKinds.def
393 ↗(On Diff #148637)

That's the goal. Just disabled in c++ for now with the end goal of getting c++ support. Right now I'm waiting for these types to be added in the next Itanium ABI, which I believe you responded to on Github. Afterwards I'll need to request Microsoft mangling.

lib/Basic/TargetInfo.cpp
45 ↗(On Diff #148445)

The integral and fractional bits will be set in the target and is available in a later patch.

45 ↗(On Diff #148445)

You're right. I was stuck in the mindset that we would be providing an integral and fractional value.

leonardchan added inline comments.May 31 2018, 9:25 AM
lib/Basic/TargetInfo.cpp
45 ↗(On Diff #148445)

The integral and fractional bits will be set in the target and is available in a later patch.

I mean just the fractional bits since the integral will just be the bit width minus fractional.

rsmith added inline comments.May 31 2018, 11:22 AM
lib/Basic/TargetInfo.cpp
45 ↗(On Diff #148445)

You're assuming that all bits will be either integral or fractional bits (and in particular that there are no padding bits). I don't know if that's actually going to be true for all target ABIs, but I suppose it's OK to make this assumption until it's proven wrong by some actual target.

ebevhan added inline comments.Jun 1 2018, 12:22 AM
lib/Basic/TargetInfo.cpp
45 ↗(On Diff #148445)

It is actually the case for us (downstream) that the unsigned types should have one bit of padding, however, this is a special case. The spec says "Each unsigned fract type has either the same number of fractional bits as, or one more fractional bit than, its corresponding signed fract type." and also under 'recommended practice', "Each signed accum type has the same number of fractional bits as either its corresponding signed fract type or its corresponding unsigned fract type."

So I think the approach would be that the integral bits are implied from the fractional bits and the width, except in the unsigned case where the MSB is a padding bit. If there is a target that really does want the unsigned types to have an extra bit of precision it could be added as a flag, but I don't really see the benefit of it. The consistency benefit of having the unsigned types have the same scaling factor as the signed ones outweighs the downsides.

Hi all, I'll be attempting to commit this patch around 6pm PT today unless anyone has any more comments on this specific patch. Any other suggestions regarding potential design changes can be discussed in future patches since this is only the first of many.

This revision was automatically updated to reflect the committed changes.