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

zahiraam · 2021-03-21T15:44:00Z

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::max_concurrency(n)]]

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

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

Signed-off-by: Zahira Ammarguellat <[email protected]>

clang/include/clang/Basic/Attr.td

clang/lib/Sema/SemaDeclAttr.cpp

Signed-off-by: Zahira Ammarguellat <[email protected]>

clang/lib/Sema/SemaDeclAttr.cpp

clang/test/CodeGenSYCL/max-concurrency.cpp

clang/test/SemaSYCL/intel-fpga-loops.cpp

clang/test/SemaSYCL/max-concurrency.cpp

clang/test/CodeGenSYCL/max-concurrency.cpp

Signed-off-by: Zahira Ammarguellat <[email protected]>

clang/lib/Sema/SemaSYCL.cpp

AaronBallman

LGTM!

clang/lib/Sema/SemaDeclAttr.cpp

smanna12 · 2021-03-29T14:26:59Z

@zahiraam, could you please update summary since we are not naming this attribute to component_max_concurrency?

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

clang/test/SemaSYCL/max-concurrency.cpp

Signed-off-by: Zahira Ammarguellat <[email protected]>

clang/lib/Sema/SemaDeclAttr.cpp

clang/include/clang/Basic/AttrDocs.td

Signed-off-by: Zahira Ammarguellat <[email protected]>

clang/lib/CodeGen/CodeGenFunction.cpp

Signed-off-by: Zahira Ammarguellat <[email protected]>

AaronBallman · 2021-03-31T17:26:47Z

LGTM, but the review is still marked as a draft.

zahiraam · 2021-03-31T17:28:58Z

LGTM, but the review is still marked as a draft.

So that's my next challenge. Not sure how to make it a "real" PR.

erichkeane · 2021-03-31T17:31:09Z

@zahiraam : https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-stage-of-a-pull-request

I did it for you :)

zahiraam · 2021-03-31T17:32:31Z

@zahiraam : https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-stage-of-a-pull-request

I did it for you :)

Wow! That was fast. I was still reading the doc. Thanks.

AaronBallman

LGTM more officially now. :-D

premanandrao · 2021-03-31T18:03:54Z

clang/include/clang/Basic/AttrDocs.td

-be applied multiple times to the same loop.
+This attribute applies to a loop or a function. Indicates that the loop/function
+should allow no more than N threads or iterations to execute it simultaneously.
+N must be a non negative integer. '0' indicates the max_concurrency case to b


premanandrao · 2021-03-31T18:06:39Z

clang/lib/Sema/SemaDeclAttr.cpp

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


This should be ArgVal < 0, I think (or the document is wrong). 0 indicates unbounded concurrency.

clang/lib/Sema/SemaDeclAttr.cpp

premanandrao · 2021-03-31T18:13:43Z

clang/test/SemaSYCL/max-concurrency.cpp

+
+// expected-error@+1 {{'max_concurrency' attribute takes one argument}}
+[[intel::max_concurrency(3, 3)]] void goo() {}
+


Please add a test for '0'.

Signed-off-by: Zahira Ammarguellat <[email protected]>

premanandrao · 2021-03-31T19:54:27Z

clang/lib/Sema/SemaDeclAttr.cpp

+      return;
+    E = Res.get();
+
+    // This attribute requires a strictly positive value.


Very minor: comment is incorrect. We need a non-negative value.

But otherwise looks good to me.

Signed-off-by: Zahira Ammarguellat <[email protected]>

premanandrao

LGTM

zahiraam added 7 commits March 21, 2021 11:42

Adding max-concurrency attr - Scratch PR.

0d2c90b

Implementing max-attribute.

8140d90

Fix indentation

8ee951b

Signed-off-by: Zahira Ammarguellat <[email protected]>

Fix indentation

1a4293d

Signed-off-by: Zahira Ammarguellat <[email protected]>

Fix indentation

301f83d

Signed-off-by: Zahira Ammarguellat <[email protected]>

Fix indentation

92f2be7

Signed-off-by: Zahira Ammarguellat <[email protected]>

Fix indentation

32f6908

Signed-off-by: Zahira Ammarguellat <[email protected]>

zahiraam changed the title ~~Adding max-concurrency attr - Scratch PR.~~ [SYCL][FPGA] Allowing max-concurrency attribute on functions. Mar 23, 2021

Change Diag's name

284a29a

Signed-off-by: Zahira Ammarguellat <[email protected]>

zahiraam requested review from AaronBallman and elizabethandrews March 23, 2021 21:08

AaronBallman reviewed Mar 24, 2021

View reviewed changes

Addressed comments from review

39a2f45

Signed-off-by: Zahira Ammarguellat <[email protected]>

AaronBallman reviewed Mar 24, 2021

View reviewed changes

zahiraam requested a review from erichkeane March 24, 2021 22:08

erichkeane reviewed Mar 25, 2021

View reviewed changes

clang/test/CodeGenSYCL/max-concurrency.cpp Show resolved Hide resolved

clang/test/CodeGenSYCL/max-concurrency.cpp Show resolved Hide resolved

zahiraam added 2 commits March 26, 2021 10:31

Addressed comments from review

cb1eb37

Signed-off-by: Zahira Ammarguellat <[email protected]>

Merge remote-tracking branch 'remote/sycl' into max-attr-scratch

2428add

zahiraam requested a review from smanna12 March 29, 2021 12:41

smanna12 reviewed Mar 29, 2021

View reviewed changes

clang/lib/Sema/SemaSYCL.cpp Outdated Show resolved Hide resolved

zahiraam added 2 commits March 29, 2021 08:48

Merge remote-tracking branch 'remote/sycl' into max-attr-scratch

01123e0

Fixing case statement.

a923b5c

AaronBallman previously approved these changes Mar 29, 2021

View reviewed changes

smanna12 reviewed Mar 29, 2021

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

smanna12 reviewed Mar 29, 2021

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Show resolved Hide resolved

smanna12 reviewed Mar 29, 2021

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

smanna12 reviewed Mar 29, 2021

View reviewed changes

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp Outdated Show resolved Hide resolved

smanna12 reviewed Mar 29, 2021

View reviewed changes

clang/test/SemaSYCL/max-concurrency.cpp Show resolved Hide resolved

Fixes for comments.

a0c8ae4

Signed-off-by: Zahira Ammarguellat <[email protected]>

Fix LIT test max-concurrency.cpp

b681e86

Signed-off-by: Zahira Ammarguellat <[email protected]>

smanna12 reviewed Mar 30, 2021

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

smanna12 reviewed Mar 30, 2021

View reviewed changes

clang/include/clang/Basic/AttrDocs.td Show resolved Hide resolved

zahiraam added 3 commits March 31, 2021 09:29

Remove optional argument for max-concurrency

fbd9a03

Signed-off-by: Zahira Ammarguellat <[email protected]>

Merge remote-tracking branch 'remote/sycl' into max-attr-scratch

350744a

fix conflict

cdfa7a3

Signed-off-by: Zahira Ammarguellat <[email protected]>

AaronBallman reviewed Mar 31, 2021

View reviewed changes

clang/lib/CodeGen/CodeGenFunction.cpp Outdated Show resolved Hide resolved

clang/lib/CodeGen/CodeGenFunction.cpp Outdated Show resolved Hide resolved

Indentation

447ca30

Signed-off-by: Zahira Ammarguellat <[email protected]>

erichkeane marked this pull request as ready for review March 31, 2021 17:30

erichkeane requested a review from premanandrao as a code owner March 31, 2021 17:30

AaronBallman previously approved these changes Mar 31, 2021

View reviewed changes

smanna12 previously approved these changes Mar 31, 2021

View reviewed changes

premanandrao reviewed Mar 31, 2021

View reviewed changes

zahiraam added 2 commits March 31, 2021 15:34

Fixes after Prem's review

af4f734

Signed-off-by: Zahira Ammarguellat <[email protected]>

Merge remote-tracking branch 'remote/sycl' into max-attr-scratch

47f0c4a

zahiraam dismissed stale reviews from smanna12 and AaronBallman via 47f0c4a March 31, 2021 19:35

premanandrao reviewed Mar 31, 2021

View reviewed changes

zahiraam added 3 commits March 31, 2021 15:57

Fix comment

c8df3cf

Signed-off-by: Zahira Ammarguellat <[email protected]>

Fix LIT test

b4e0e93

Signed-off-by: Zahira Ammarguellat <[email protected]>

Merge remote-tracking branch 'remote/sycl' into max-attr-scratch

5357212

premanandrao approved these changes Apr 1, 2021

View reviewed changes

smanna12 approved these changes Apr 1, 2021

View reviewed changes

bader merged commit ec80421 into intel:sycl Apr 1, 2021

bader mentioned this pull request Apr 1, 2021

[SYCL] Remove manual diagnostic checking for stmt attrs. #3442

Merged

smanna12 mentioned this pull request Jun 1, 2021

[SYCL] Implement SYCL 2020 spec functionality: no propagation from functions to the caller #3836

Closed


		// expected-error@+1 {{'max_concurrency' attribute takes one argument}}
		[[intel::max_concurrency(3, 3)]] void goo() {}

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smanna12 commented Mar 29, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AaronBallman commented Mar 31, 2021

Uh oh!

zahiraam commented Mar 31, 2021

Uh oh!

erichkeane commented Mar 31, 2021

Uh oh!

zahiraam commented Mar 31, 2021

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

premanandrao Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

premanandrao Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

premanandrao Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

premanandrao Mar 31, 2021

Choose a reason for hiding this comment

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zahiraam commented Mar 21, 2021 •

edited

Loading