Page MenuHomePhabricator

Implement _ExtInt as an extended int type specifier.
Needs ReviewPublic

Authored by erichkeane on Tue, Feb 4, 7:08 AM.

Details

Summary

Introduction/Motivation:
LLVM-IR supports integers of non-power-of-2 bitwidth, in the iN syntax.
Integers of non-power-of-two aren't particularly interesting or useful
on most hardware, so much so that no language in Clang has been
motivated to expose it before.

However, in the case of FPGA hardware normal integer types where the
full bitwidth isn't used, is extremely wasteful and has severe
performance/space concerns. Because of this, Intel has introduced this
functionality in the High Level Synthesis compiler[0]
under the name "Arbitrary Precision Integer" (ap_int for short). This
has been extremely useful and effective for our users, permitting them
to optimize their storage and operation space on an architecture where
both can be extremely expensive.

We are proposing upstreaming a more palatable version of this to the
community, in the form of this proposal and accompanying patch. We are
proposing the syntax _ExtInt(N). We intend to propose this to the WG14
committee[1], and the underscore-capital seems like the active direction
for a WG14 paper's acceptance. An alternative that Richard Smith
suggested on the initial review was __int(N), however we believe that
is much less acceptable by WG14. We considered _Int, however _Int is
used as an identifier in libstdc++ and there is no good way to fall
back to an identifier (since _Int(5) is indistinguishable from an
unnamed initializer of a template type named _Int).

[0]https://www.intel.com/content/www/us/en/software/programmable/quartus-prime/hls-compiler.html)
[1]http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2472.pdf

Diff Detail

Event Timeline

erichkeane created this revision.Tue, Feb 4, 7:08 AM
rsmith added a subscriber: rsmith.Wed, Feb 5, 6:27 PM

Have you considered how this would interact with our other language extensions? Can we form a vector of _ExtInt(N)? A _Complex _ExtInt(N)? I expect for some of that to just fall out of the implementation, but we should have documentation and test coverage either way.

I don't see any test coverage for _Atomic(_ExtInt(N)), which may not fall out from the other work because (if memory serves) atomic lowering doesn't go through the normal ConvertTypeForMem path and instead deals with padding itself.

I would like to see your documentation cover the calling convention used when passing/returning these types by value.

clang/lib/AST/ItaniumMangle.cpp
3472

This is not a valid vendor-extension mangling. There are two choices here, per the current scheme:

  1. mangle as a type, using (lowercase) u followed by a source-name, such as u9_ExtInt17 / u10_UExtInt17
  2. mangle as a type qualifier, using (capital) U followed by a source-name and optional template-args, such as U7_ExtIntILi17EEi / U7_ExtIntILi17EEj

Neither of these gives a particularly nice demangling. If WG14 seems likely to accept the proposal, you should register a proper mangling as part of the Itanium ABI.

keryell added a subscriber: keryell.Thu, Feb 6, 7:12 AM
erichkeane marked an inline comment as done.Thu, Feb 6, 7:34 AM

Have you considered how this would interact with our other language extensions? Can we form a vector of _ExtInt(N)? A _Complex _ExtInt(N)? I expect for some of that to just fall out of the implementation, but we should have documentation and test coverage either way.

I don't see any test coverage for _Atomic(_ExtInt(N)), which may not fall out from the other work because (if memory serves) atomic lowering doesn't go through the normal ConvertTypeForMem path and instead deals with padding itself.

I would like to see your documentation cover the calling convention used when passing/returning these types by value.

We can do a std::vector or any of the standard containers with no problems.

Extended-vector types don't really make sense for non-powers-of-two (plus have some odd gotchas when it comes to vectors of i1 for example), so I've added a test that shows that this is rejected.

_Complex _ExtInt(N) is rejected when parsing _Complex. It doesn't seem to make sense to me to support them, so I've added a test that shows it is invalid. Do you disagree?

_Atomic seems to be broken (atomic memory access size mus be byte-sized), but I'll continue to work on it to update this patch further.

I've also asked a coworker to better describe the calling convention so that we can add that text to the language-extensions.

Thanks!

clang/lib/AST/ItaniumMangle.cpp
3472

Well shucks, I was so close :) Looks like I'm just missing the last E/i from the second one.

The second seems consistently changable with the one below (for a dependent version), so I think I'm stuck with that. It DOES show up a little weird unfortunately, but at least now it demangles.

I'll ask about an official mangling mechanism for ItaniumABI.

erichkeane updated this revision to Diff 242901.Thu, Feb 6, 7:37 AM

Go through @rsmith 's comments. Current opens:

1- Getting an official mangling. This will likely need to wait until WG14 has seen the paper, in the meantime, use @rsmith suggested version.

2- _Atomic _ExtInt creates invalid IR. Patch to fix this is WIP.

3- Document Calling Convention in Language Extensions. Text is WIP.

4- Determine whether implicit conversions between these types should be acceptable. Pending discussion on CFE-dev.

martong resigned from this revision.Thu, Feb 6, 7:42 AM

Deal with _Atomic. There isn't really a sensible way to do _Atomics on all platforms for these types, so this patch now limits to the LLVM atomic instruction limits, which is powers-of-2 >=8.

Extended-vector types don't really make sense for non-powers-of-two (plus have some odd gotchas when it comes to vectors of i1 for example), so I've added a test that shows that this is rejected.

OK, that seems fine to me.

_Complex _ExtInt(N) is rejected when parsing _Complex. It doesn't seem to make sense to me to support them, so I've added a test that shows it is invalid. Do you disagree?

We allow _Complex T for integral types T in general, so this seems inconsistent. Are there problems supporting this? Is it just a parser limitation or something deeper? If there's some reason it doesn't naturally work (as there is for at least non-power-of-two-sized integers in vectors) then rejecting it seems fine.

We allow _Complex T for integral types T in general, so this seems inconsistent. Are there problems supporting this? Is it just a parser limitation or something deeper? If there's some reason it doesn't naturally work (as there is for at least non-power-of-two-sized integers in vectors) then rejecting it seems fine.

At the moment it doesn't work because of parsing. I'm unaware of there is a deeper limitation, but I'll look at it. I was unaware that we allow integral types for _Complex, I'd looked it up on cppreference and only saw the FP types so I was basically just confused as to what your intent was. I'll look closer now that I've done the _Atomic limitations.

At the moment it doesn't work because of parsing. I'm unaware of there is a deeper limitation, but I'll look at it. I was unaware that we allow integral types for _Complex, I'd looked it up on cppreference and only saw the FP types so I was basically just confused as to what your intent was.

Nice! Telecommunication applications love complex numbers of weird integers on FPGA. Of course, it is outside of any common CPU ABI but useful for HLS tool-chains based on Clang. Useful also for SYCL extensions for FPGA.

Add _Complex support for these types. As it turns out, they 'just work'! I added a check to the same place we check 'int' in the declaration specifiers, and the IR that gets generated is eminently logical.

Fixed test that expected _Complex _ExtInt to fail.

Remove conversions between _ExtInt until WG14 decides what should be done with them.