[SYCL][FPGA] Allowing max-concurrency attribute on functions. #3362

zahiraam · 2021-03-15T20:05:03Z

This patch implements the support of a new FPGA function attribute ‘max_concurrency”. The attribute exists already for loops. It takes a single unsigned integer argument and has the following syntax:
[[intel::component_max_concurrency(n)]]

An example of it use:
[[intel::component_max_concurrency(n)]]
void foo() {
}
The LLVM IR representation will be function metadata:
!component_max_concurrency !0
!0 = !{!i32 n}

Max_concurrency applies to functions in device code. It is not be propagated to the caller.

smanna12 · 2021-03-16T02:14:34Z

clang/test/CodeGenSYCL/max-concurrency.cpp

+template <int NT>
+[[intel::reqd_sub_group_size(NT)]] void func() {}


Did you mean to use [[intel::max_concurrency(NT)]] here?

smanna12 · 2021-03-16T02:23:04Z

clang/include/clang/Basic/AttrDocs.td

    [[intel::max_concurrency(2)]] for (int i = 0; i != 10; ++i) a[i] = 0;
  }

+  [[intel::component_max_concurrency(2)]] void foo1 { }


You are adding new spelling here. You did not add it inside Attr.td file and also CodeGen test is using existing loop attribute spelling [[intel::max_concurrency()]]. Are we going to use same loop attribute spelling as function attribute?

smanna12 · 2021-03-16T02:24:38Z

clang/test/CodeGenSYCL/max-concurrency.cpp

+// CHECK: !17 = !{i32 2}
+// CHECK: !18 = !{i32 3}
+
+template <typename name, typename Func>


Please use updated guidelines for new FE tests:
Please refer section “Guidelines for adding DPC++ in-tree LIT tests (DPC++ Clang FE tests)” in the https://github.com/intel/llvm/blob/sycl/CONTRIBUTING.md guide for the suggested guidelines

smanna12 · 2021-03-16T02:29:50Z

clang/lib/CodeGen/CodeGenFunction.cpp

+
+  if (const SYCLIntelFPGAMaxConcurrencyAttr *A =
+          FD->getAttr<SYCLIntelFPGAMaxConcurrencyAttr>()) {
+    const auto *CE = dyn_cast<ConstantExpr>(A->getNThreadsExpr());


you are missing an assert here before ArgVal.
assert(CE && "Not an integer constant expression");

Suggested change

const auto *CE = dyn_cast<ConstantExpr>(A->getNThreadsExpr());

const auto *CE = cast<ConstantExpr>(A->getNThreadsExpr());

No need for a dyn_cast<> followed by an assert.

smanna12 · 2021-03-16T03:12:45Z

clang/include/clang/Basic/AttrDocs.td

  void bar() {
    [[intel::max_concurrency(N)]] for(;;) { }
  }
+  [[intel::component_max_concurrency(N)]] void bar1() { }


same here, new attribute spelling, you did not use this spelling

smanna12 · 2021-03-16T03:14:44Z

clang/lib/Sema/SemaSYCL.cpp

+      if (auto *A = FD->getAttr<SYCLIntelFPGAMaxConcurrencyAttr>())
+        Attrs.insert(A);


Are we going to propagate this attribute to the caller?

smanna12 · 2021-03-16T03:16:07Z

Could you please provide description about this PR? Also need SemaSYCL test here. Thanks.

AaronBallman

All of the sema tests are missing for testing the warning situations (applies to functions and not other types of declarations, etc).

I think we're also missing a "merge" function for the attribute as functions can have redeclarations. e.g., we want to catch things like:

[[intel::max_concurrency(4)]] void func();
[[intel::max_concurrency(5)]] void func() {}

AaronBallman · 2021-03-16T11:34:29Z

clang/include/clang/Basic/Attr.td

                   CXX11<"intel","max_concurrency">];
-  let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt],
+  let Subjects = SubjectList<[ForStmt, CXXForRangeStmt, WhileStmt, DoStmt, Function],
                             ErrorDiag, "'for', 'while', and 'do' statements">;


The text of the diagnostic needs to be updated as well.

AaronBallman · 2021-03-16T11:35:23Z

clang/include/clang/Basic/Attr.td

 }

-def SYCLIntelFPGAMaxConcurrency : StmtAttr {
+def SYCLIntelFPGAMaxConcurrency : InheritableAttr {


Suggested change

def SYCLIntelFPGAMaxConcurrency : InheritableAttr {

def SYCLIntelFPGAMaxConcurrency : DeclOrStmtAttr {

AaronBallman · 2021-03-16T11:36:39Z

clang/include/clang/Sema/Sema.h

  /// declaration.
  void addSYCLIntelPipeIOAttr(Decl *D, const AttributeCommonInfo &CI, Expr *ID);

+  /// AddSYCLIntelFPGAMaxConcurrencyAttr - Adds a max_component attribute to a


max_component -- did you mean max_concurrency?

AaronBallman · 2021-03-16T11:37:33Z

clang/lib/CodeGen/CodeGenFunction.cpp

+
+  if (const SYCLIntelFPGAMaxConcurrencyAttr *A =
+          FD->getAttr<SYCLIntelFPGAMaxConcurrencyAttr>()) {
+    const auto *CE = dyn_cast<ConstantExpr>(A->getNThreadsExpr());


Suggested change

const auto *CE = dyn_cast<ConstantExpr>(A->getNThreadsExpr());

const auto *CE = cast<ConstantExpr>(A->getNThreadsExpr());

No need for a dyn_cast<> followed by an assert.

AaronBallman · 2021-03-16T11:37:53Z

clang/lib/CodeGen/CodeGenFunction.cpp

    Fn->setMetadata("stall_enable", llvm::MDNode::get(Context, AttrMDArgs));
  }
+
+  if (const SYCLIntelFPGAMaxConcurrencyAttr *A =


Suggested change

if (const SYCLIntelFPGAMaxConcurrencyAttr *A =

if (const auto *A =

Because the type is spelled out explicitly in the initializer, this is a good time to use auto.

AaronBallman · 2021-03-16T11:39:45Z

clang/lib/CodeGen/CodeGenFunction.cpp

+  if (const SYCLIntelFPGAMaxConcurrencyAttr *A =
+          FD->getAttr<SYCLIntelFPGAMaxConcurrencyAttr>()) {
+    const auto *CE = dyn_cast<ConstantExpr>(A->getNThreadsExpr());
+    Optional<llvm::APSInt> ArgVal = CE->getResultAsAPSInt();


Suggested change

Optional<llvm::APSInt> ArgVal = CE->getResultAsAPSInt();

llvm::APSInt ArgVal = CE->getResultAsAPSInt();

The function doesn't return an Optional, so I assume this was unintentional.

zahiraam · 2021-03-17T20:43:57Z

All of the sema tests are missing for testing the warning situations (applies to functions and not other types of declarations, etc).

I think we're also missing a "merge" function for the attribute as functions can have redeclarations. e.g., we want to catch things like:
[[intel::max_concurrency(4)]] void func();
[[intel::max_concurrency(5)]] void func() {}

@AaronBallman Forgot to work on this. Sorry.
I will do that now.

smanna12 · 2021-03-17T20:49:44Z

clang/lib/Sema/SemaSYCL.cpp

+        case attr::Kind::SYCLIntelFPGAMaxConcurrency: {
+          auto *SIMCA = cast<SYCLIntelFPGAMaxConcurrencyAttr>(A);
+          if (auto *Existing =
+                  SYCLKernel->getAttr<SYCLIntelFPGAMaxConcurrencyAttr>()) {
+            ASTContext &Ctx = getASTContext();
+            if (Existing->getNThreadsExpr() > SIMCA->getNThreadsExpr()) {
+              Diag(SYCLKernel->getLocation(),
+                   diag::err_conflicting_sycl_kernel_attributes);
+              Diag(Existing->getLocation(), diag::note_conflicting_attribute);
+              Diag(SIMCA->getLocation(), diag::note_conflicting_attribute);
+              SYCLKernel->setInvalidDecl();
+            } else {
+              SYCLKernel->addAttr(A);
+            }
+          } else {
+            SYCLKernel->addAttr(A);
+          }
+          break;
+        }


i am wondering what is the purpose of adding this diagnostic here? I do not see any test regarding this?

I added to be able to check on things like this, previous attribute on a function.
[[intel::max_concurrency(4)]] void func();
[[intel::max_concurrency(5)]] void func() {}
I need to add a test case for this.

You can add "merge" function to check this instead of SemaSYCL codes here:
SYCLIntelFPGAMaxConcurrencyAttr *Sema::MergeSSYCLIntelFPGAMaxConcurrencyAttr(
Decl *D, const SYCLIntelFPGAMaxConcurrencyAttr &A) {

Ok. Studying that! Thanks.

I think adding the code here only handles diagnostics when applying attributes to SYCL kernel i.e. when attributes are propagated to kernel from device functions called by kernel. I don't think duplicate attributes on any other function will be handled by this

Based on code in L563, it looks like while the attribute can be explicitly specified on kernel, it is not propagated to kernel from device functions. In this case, I do not think this block of code is required. I think the "merge" function as @smanna12 should cover kernel function as well. However please verify with tests for 'normal' and kernel functions.

premanandrao · 2021-03-23T18:07:36Z

clang/include/clang/Basic/AttrDocs.td

  void bar() {
    [[intel::max_concurrency(N)]] for(;;) { }
  }
+  [[intel::max_concurrency(N)]] void bar1() { }


This example is missing the template keyword that introduces N.

premanandrao · 2021-03-23T18:11:31Z

clang/lib/Sema/SemaDeclAttr.cpp

+    E = Res.get();
+
+    // This attribute requires a strictly positive value.
+    if (ArgVal <= 0) {


Document description says non-negative values are allowed, and that zero means unbounded.

premanandrao · 2021-03-23T18:15:33Z

clang/test/CodeGenSYCL/max-concurrency.cpp

@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -disable-llvm-passes -triple spir64-unknown-unknown-sycldevice -sycl-std=2020 -emit-llvm -o - %s | FileCheck %s
+


Please add a comment here describing what the test does.

premanandrao · 2021-03-23T18:20:05Z

clang/test/CodeGenSYCL/max-concurrency.cpp

+int main() {
+  queue q;
+
+  kernel_single_task_1<class kernel_function>([]() {


New guidelines suggest using kernel_single_task from the sycl.hpp header instead.

premanandrao · 2021-03-23T18:20:56Z

clang/test/CodeGenSYCL/max-concurrency.cpp

+      func<2>();
+    });
+
+  });


Could you also add an example that uses '0'?

premanandrao · 2021-03-23T18:21:24Z

clang/test/SemaSYCL/max-concurrency.cpp

@@ -0,0 +1,125 @@
+// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -sycl-std=2020  -fsyntax-only -ast-dump -verify -pedantic %s | FileCheck %s
+


Please add a comment here indicating what this test does.

premanandrao · 2021-03-23T18:23:46Z

clang/test/SemaSYCL/max-concurrency.cpp

+
+[[intel::max_concurrency(8)]] void dup();
+[[intel::max_concurrency(9)]] void dup() {} // expected-error {{duplicate Intel FPGA function attribute 'max_concurrency'}}
+


Could you also add an example where the declaration and definition use the same value?

d54f77c5 ("[NFC] Split of SPT and SPIR-V in header parsing (#2316)", 2024-03-11) made a copy of the error log, with the presumably unintended consequence that errors are no longer propagated back to the SPIRVModule itself. Original commit: KhronosGroup/SPIRV-LLVM-Translator@3d58c69cf2f3704

[SYCL][FPGA] Allowing max-concurrency attribute on functions.

ad38dc6

zahiraam requested review from elizabethandrews and premanandrao as code owners March 15, 2021 20:05

smanna12 reviewed Mar 16, 2021

View reviewed changes

smanna12 requested a review from AaronBallman March 16, 2021 03:23

AaronBallman requested changes Mar 16, 2021

View reviewed changes

Responded to all comments in review.

4129be1

smanna12 reviewed Mar 17, 2021

View reviewed changes

Responded to all comments in review.

86352d3

premanandrao reviewed Mar 23, 2021

View reviewed changes

zahiraam closed this Mar 23, 2021

		template <int NT>
		[[intel::reqd_sub_group_size(NT)]] void func() {}

	const auto *CE = dyn_cast<ConstantExpr>(A->getNThreadsExpr());
	const auto *CE = cast<ConstantExpr>(A->getNThreadsExpr());

		if (auto *A = FD->getAttr<SYCLIntelFPGAMaxConcurrencyAttr>())
		Attrs.insert(A);

	def SYCLIntelFPGAMaxConcurrency : InheritableAttr {
	def SYCLIntelFPGAMaxConcurrency : DeclOrStmtAttr {

	if (const SYCLIntelFPGAMaxConcurrencyAttr *A =
	if (const auto *A =

	Optional<llvm::APSInt> ArgVal = CE->getResultAsAPSInt();
	llvm::APSInt ArgVal = CE->getResultAsAPSInt();

		@@ -0,0 +1,89 @@
		// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -disable-llvm-passes -triple spir64-unknown-unknown-sycldevice -sycl-std=2020 -emit-llvm -o - %s \| FileCheck %s

		@@ -0,0 +1,125 @@
		// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -sycl-std=2020 -fsyntax-only -ast-dump -verify -pedantic %s \| FileCheck %s


		[[intel::max_concurrency(8)]] void dup();
		[[intel::max_concurrency(9)]] void dup() {} // expected-error {{duplicate Intel FPGA function attribute 'max_concurrency'}}

[SYCL][FPGA] Allowing max-concurrency attribute on functions. #3362

[SYCL][FPGA] Allowing max-concurrency attribute on functions. #3362

Uh oh!

Conversation

zahiraam commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smanna12 Mar 16, 2021 • 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!

smanna12 Mar 16, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smanna12 Mar 16, 2021 • 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!

smanna12 Mar 16, 2021 • 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!

smanna12 commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zahiraam commented Mar 17, 2021

Uh oh!

smanna12 Mar 17, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

smanna12 Mar 17, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zahiraam commented Mar 15, 2021 •

edited

Loading

smanna12 Mar 16, 2021 •

edited

Loading

smanna12 Mar 16, 2021 •

edited

Loading

smanna12 Mar 16, 2021 •

edited

Loading

smanna12 Mar 16, 2021 •

edited

Loading

smanna12 commented Mar 16, 2021 •

edited

Loading

smanna12 Mar 17, 2021 •

edited

Loading

smanna12 Mar 17, 2021 •

edited

Loading