Implement _ExtInt as an extended int type specifier. #1067

erichkeane · 2020-01-28T16:15:12Z

NOTE: We will proposing this patch to LLVM community in another month or
two, however I've been instructed that this is a very valuable patch for
our SYCL downstreams and have been encouraged to submit this for SYCL as
a 'preview'.

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
(https://www.intel.com/content/www/us/en/software/programmable/quartus-prime/hls-compiler.html)
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, 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).

Signed-off-by: Erich Keane [email protected]

NOTE: We will proposing this patch to LLVM community in another month or two, however I've been instructed that this is a very valuable patch for our SYCL downstreams and have been encouraged to submit this for SYCL as a 'preview'. 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 (https://www.intel.com/content/www/us/en/software/programmable/quartus-prime/hls-compiler.html) 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, 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). Signed-off-by: Erich Keane <[email protected]>

GarveyJoe · 2020-01-31T23:34:30Z

clang/test/SemaCXX/ext-int.cpp

+  static_assert(is_same<decltype(x32_s * x_int), int>::value,"");
+  static_assert(is_same<decltype(x32_u / x_int), unsigned int>::value,"");
+  static_assert(is_same<decltype(x32_s * x_uint), unsigned int>::value,"");
+  static_assert(is_same<decltype(x32_u / x_uint), unsigned int>::value,"");


Maybe I missed it but you probably want a case with a char mixed with an ExtInt of size < 32 because it's a weird special case where we don't promote all the way to int.

MrSidims

Please add AST tests for both type. I really want to see how it fits together.

MrSidims · 2020-02-13T17:16:21Z

clang/include/clang/AST/Type.h

@@ -6162,6 +6163,63 @@ class PipeType : public Type, public llvm::FoldingSetNode {
  bool isReadOnly() const { return isRead; }
 };

+// A fixed int type of a specified bitwidth.


I feel like it's possible to define only DependentExtIntType class here. But it will come in cost of extra isInstantiationDependent() in the code.

MrSidims · 2020-02-13T17:18:47Z

clang/include/clang/AST/Type.h

@@ -6681,6 +6739,10 @@ inline bool Type::isPipeType() const {
  return isa<PipeType>(CanonicalType);
 }

+inline bool Type::isExtIntType() const {
+  return isa<ExtIntType>(CanonicalType);


Suggested change

return isa<ExtIntType>(CanonicalType);

return isa<ExtIntType>(CanonicalType) || isa<DependentExtIntType>(CanonicalType);

MrSidims · 2020-02-13T17:23:34Z

clang/lib/AST/ASTContext.cpp

@@ -5822,6 +5867,12 @@ int ASTContext::getFloatingTypeSemanticOrder(QualType LHS, QualType RHS) const {
 unsigned ASTContext::getIntegerRank(const Type *T) const {
  assert(T->isCanonicalUnqualified() && "T should be canonicalized");

+  // Results in this 'losing' to any type of the same size, but winning if
+  // larger.
+  if (const auto *EIT = dyn_cast<ExtIntType>(T)) {


Ah, I probably got, why two types and not 1 are yet required.

MrSidims · 2020-02-13T17:24:27Z

clang/lib/AST/MicrosoftMangle.cpp

+                                         Qualifiers, SourceRange Range) {
+  DiagnosticsEngine &Diags = Context.getDiags();
+  unsigned DiagID = Diags.getCustomDiagID(
+      DiagnosticsEngine::Error, "cannot mangle this DependentExtInt type yet");


I didn't get why. Can you please clarify?

MrSidims · 2020-02-13T17:28:10Z

clang/lib/AST/Type.cpp

@@ -1936,6 +1963,9 @@ bool Type::isSignedIntegerType() const {
      return ET->getDecl()->getIntegerType()->isSignedIntegerType();
  }

+  if (const ExtIntType *IT = dyn_cast<ExtIntType>(CanonicalType))


Probably here and after checks for DependentExtIntType are missed.

bader · 2020-02-24T15:37:41Z

Also being reviewed here: https://reviews.llvm.org/D73967.
@erichkeane, should we close this PR and leave comments in LLVM Phabricator?

erichkeane · 2020-02-24T15:39:05Z

We probably shouldn't close this just yet since I was told that this is vital to get into SYCL for FPGA, so in case it doesn't survive community review we can bring it back here, but feel free to leave comments in phab.

erichkeane · 2020-03-18T16:41:27Z

We satisfied the FPGA folks via a different mechanism downstream, so reviews to this should go here: https://reviews.llvm.org/D73967

PLEASE anyone who has feedback/can help get that in, please help by reviewing (or approving :) ) that patch in LLVM.

Closing this as a result.

erichkeane requested review from bader, Fznamznon, GarveyJoe and elizabethandrews January 28, 2020 16:15

GarveyJoe reviewed Jan 31, 2020

View reviewed changes

MrSidims reviewed Feb 13, 2020

View reviewed changes

erichkeane closed this Mar 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement _ExtInt as an extended int type specifier. #1067

Implement _ExtInt as an extended int type specifier. #1067

Uh oh!

erichkeane commented Jan 28, 2020

Uh oh!

GarveyJoe Jan 31, 2020

Uh oh!

MrSidims left a comment

Uh oh!

MrSidims Feb 13, 2020

Uh oh!

MrSidims Feb 13, 2020

Uh oh!

MrSidims Feb 13, 2020

Uh oh!

MrSidims Feb 13, 2020

Uh oh!

MrSidims Feb 13, 2020 •

edited

Loading

Uh oh!

bader commented Feb 24, 2020

Uh oh!

erichkeane commented Feb 24, 2020

Uh oh!

erichkeane commented Mar 18, 2020

Uh oh!

Uh oh!

	return isa<ExtIntType>(CanonicalType);
	return isa<ExtIntType>(CanonicalType) \|\| isa<DependentExtIntType>(CanonicalType);

Implement _ExtInt as an extended int type specifier. #1067

Implement _ExtInt as an extended int type specifier. #1067

Uh oh!

Conversation

erichkeane commented Jan 28, 2020

Uh oh!

GarveyJoe Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

MrSidims left a comment

Choose a reason for hiding this comment

Uh oh!

MrSidims Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

MrSidims Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

MrSidims Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

MrSidims Feb 13, 2020

Choose a reason for hiding this comment

Uh oh!

MrSidims Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bader commented Feb 24, 2020

Uh oh!

erichkeane commented Feb 24, 2020

Uh oh!

erichkeane commented Mar 18, 2020

Uh oh!

Uh oh!

MrSidims Feb 13, 2020 •

edited

Loading