[SYCL] Add support for new FPGA function attribute stall_enable #2734

smanna12 · 2020-11-05T14:41:12Z

This patch adds support a new FPGA function attribute, stall_enable,
to be sent through to the backend (and ignored by the emulator).

Syntax:
[[intel::stall_enable]]

This function attribute applies to a lambda function, or function definition.
Requests, to the extent possible, that statically-scheduled clusters handle
stalls using a stall-enable signal to freeze computation within the cluster.

Stall_enable attribute can be applied to device function/kernel
and it takes no arguments.

LLVM IR function metadata should simply be i32 1.

Signed-off-by: Soumi Manna [email protected]

This patch adds support a new FPGA function attribute, stall_enable, to be sent through to the backend (and ignored by the emulator). Syntax: [[intel::stall_enable]] This function attribute applies to a lambda function, or function definition. Requests, to the extent possible, that statically-scheduled clusters handle stalls using a stall-enable signal to freeze computation within the cluster. Note that this attribute can be applied to any lambda or function definition, it does not have to be a device function/kernel. Stall_enable attribute takes no arguments and LLVM IR function metadata should simply be i32 1. Signed-off-by: Soumi Manna <[email protected]>

Signed-off-by: Soumi Manna <[email protected]>

Fznamznon

It looks ok in general except my little comment and the fact that I completely didn't get the doc, because it looks like very specific for some devices (FPGA?).

Fznamznon · 2020-11-05T15:40:15Z

clang/lib/Sema/SemaDeclAttr.cpp

+    } else if (const auto *A = D->getAttr<SYCLIntelStallEnableAttr>()) {
+      Diag(D->getLocation(), diag::err_opencl_kernel_attr) << A;
+      D->setInvalidDecl();


These lines seem not necessary. The attribute stall_enable seems to be enabled only for SYCL and this section of the code produces errors for attributes which should be applied to OpenCL kernel in OpenCL language and all these diagnostics are disabled for SYCL. See if statement on lines 8822-8823 in this file.

Suggested change

} else if (const auto *A = D->getAttr<SYCLIntelStallEnableAttr>()) {

Diag(D->getLocation(), diag::err_opencl_kernel_attr) << A;

D->setInvalidDecl();

It looks ok in general except my little comment and the fact that I completely didn't get the doc, because it looks like very specific for some devices (FPGA?).

I meant that, I didn't get the doc because I'm not familiar with FPGA devices. Does it make sense to mention that it is some FPGA-specific thing?

i looked at other new FPGA function attribute "SYCLIntelSchedulerTargetFmaxMhz" . It did not add those line (i think i can remove those).

i do not see anywhere it is mentioned to be a FPGA specific.

Previously we had FPGA specific attributes in intelfpga namespace, so it was pretty much obvious to which device they belong. Now when intelfpga was deprecated in favor of just intel namespace we can re-consider texts in some warnings etc, but it can be done in a separate patch.

I have created separate PR for this issue.

#2740

Signed-off-by: Soumi Manna <[email protected]>

AlexeySotkin · 2020-11-09T12:54:44Z

clang/test/CodeGenSYCL/stall_enable.cpp

+// CHECK: define spir_kernel void @"{{.*}}test_kernel2"() #0 {{.*}} !stall_enable [[FOO:![0-9]+]]
+// CHECK: define spir_kernel void @"{{.*}}test_kernel3"() #0 {{.*}} !stall_enable [[FOO:![0-9]+]]


Do we expect the same metadata for all 3 kernels? If yes, we can do

Suggested change

// CHECK: define spir_kernel void @"{{.*}}test_kernel2"() #0 {{.*}} !stall_enable [[FOO:![0-9]+]]

// CHECK: define spir_kernel void @"{{.*}}test_kernel3"() #0 {{.*}} !stall_enable [[FOO:![0-9]+]]

// CHECK: define spir_kernel void @"{{.*}}test_kernel2"() #0 {{.*}} !stall_enable [[FOO]]

// CHECK: define spir_kernel void @"{{.*}}test_kernel3"() #0 {{.*}} !stall_enable [[FOO]]

@AlexeySotkin, Thanks for taking a look at the patch and the feedback. All three kernels expect same metadata here. I have updated test.

Signed-off-by: Soumi Manna <[email protected]>

elizabethandrews · 2020-11-09T14:56:20Z

clang/lib/Sema/SemaSYCL.cpp

@@ -543,6 +543,9 @@ class MarkDeviceFunction : public RecursiveASTVisitor<MarkDeviceFunction> {
      if (auto *A = FD->getAttr<SYCLIntelNoGlobalWorkOffsetAttr>())
        Attrs.insert(A);

+      if (auto *A = FD->getAttr<SYCLIntelStallEnableAttr>())


This means this attribute is propagated to kernel if its applied to any function the kernel calls. Is that the requirement? If so, I think we should add this to documentation

Thanks @elizabethandrews for taking a look at the patch and the feedback.

No. The attribute can not be applied to arbitrary device functions, only to kernels.

If intel::stall_enable is applied to a function called from a device
kernel, the attribute is ignored and it is not propagated to a kernel.

The attribute must be used on a kernel lambda or functor, e.g.,:
cgh.single_task([=] () [[intel::stall_enable]]
{ // kernel body }
);

This attribute will be similar like kernel_args_restrict and scheduler_target_fmax_mhz as an example of kernel attributes.

I have updated documentation.

If intel::stall_enable is applied to a function called from a device
kernel, the attribute is ignored and it is not propagated to a kernel.

But I guess this part of the code will propagate this attribute from any function, right? We probably need to double check it.

Right. This needs to be removed if we don't want to propagate attribute.

I have updated SemaSYCL file. We will not propagate attribute on functions that are called from the kernel. I am not sure whether there is any other way we can handle this.

// Allow the kernel attribute "stall_enable" only on lambda functions
// and function objects that are called directly from a kernel
// (i.e. the one passed to the single_task or parallel_for functions).
// For all other cases, emit a warning and ignore.

elizabethandrews · 2020-11-09T15:01:32Z

clang/include/clang/Basic/AttrDocs.td

+Applies to a lambda function, or function definition. Requests, to the extent
+possible, that statically-scheduled clusters handle stalls using a stall-enable
+signal to freeze computation within the cluster. This attribute can be applied
+to device function/kernel and not to arbitrary device functions. This takes no


Can you elaborate on this? I think this line needs to be more clear. What is device function (in device function/kernel) vs 'arbitrary device functions'

I have updated documentation to be more clear about the attribute.

Signed-off-by: Soumi Manna <[email protected]>

premanandrao · 2020-11-09T20:19:03Z

clang/include/clang/Basic/Attr.td

@@ -1226,6 +1226,19 @@ def SYCLIntelNumSimdWorkItems : InheritableAttr {
  let PragmaAttributeSupport = 0;
 }

+def SYCLIntelStallEnable : InheritableAttr {
+  let Spellings = [CXX11<"intel","stall_enable">];
+  let LangOpts = [SYCLIsDevice, SYCLIsHost];


It looks like we are simply ignoring the attribute on host. Let's just allow it on device.

Yes we are ignoring it on host, but now host compiler emits warnings when sees this attribute, because it became unknown for host compiler once you removed SYCLIsHost lang option from here. I.e. there is no way to write code with this attribute without emitting warnings now, this is not good for users. Can we return SYCLIsHost back, please?

I see what you are saying @Fznamznon. I take back my suggestion. Thanks!

I have added back SYCLIsHost. Thanks for the suggestion.

premanandrao · 2020-11-09T20:23:45Z

clang/include/clang/Basic/AttrDocs.td

+  let Category = DocCatFunction;
+  let Heading = "intel::stall_enable";
+  let Content = [{
+Applies to a device function/lambda function. Requests, to the extent


I would like this changed to: "When applied to a lambda or
function call operator (of a function object) on device, this requests, to the extent ..."

premanandrao · 2020-11-09T20:26:07Z

clang/lib/Sema/SemaDeclAttr.cpp

+    return;
+
+  if (S.LangOpts.SYCLIsHost)
+    return;


Since we are not applying the attribute on host, I think we should allow it only on device.

premanandrao · 2020-11-09T20:30:00Z

clang/test/CodeGenSYCL/stall_enable.cpp

+    h.single_task<class test_kernel1>(f);
+
+    h.single_task<class test_kernel2>(
+        []() { test(); });


I am confused by this case. Is the attribute on function test(), expected to be propagated?

No, it should not be propagated.

premanandrao · 2020-11-09T20:31:29Z

clang/test/SemaSYCL/stall_enable.cpp

+  [[intel::stall_enable]] void operator()() const {}
+};
+
+[[intel::stall_enable]] void func_do_not_ignore() {}


Likewise, this one too. Is the attribute valid only on lambdas and function objects passed to the kernel or even on functions called from the kernel?

The attribute is valid only on lambdas and function objects passed to the kernel not on function called from kernel.

Signed-off-by: Soumi Manna <[email protected]>

Fznamznon · 2020-11-10T08:50:29Z

clang/include/clang/Basic/AttrDocs.td

+
+The ``intel::stall_enable`` attribute has an effect when applied to a
+function, and no effect otherwise. This attribute takes no argument and
+LLVM IR function metadata should simply be i32 1.


Do we really need to mention what will be emitted in LLVM IR?

I do not think we need to mention this. Removed it.

Signed-off-by: Soumi Manna <[email protected]>

clang/lib/Sema/SemaSYCL.cpp

Signed-off-by: Soumi Manna <[email protected]>

premanandrao · 2020-11-10T15:26:29Z

clang/test/SemaSYCL/stall_enable.cpp

+    Functor16 f16;
+    // CHECK-LABEL: FunctionDecl {{.*}}test_kernel3
+    // CHECK:       SYCLIntelStallEnableAttr {{.*}}
+    h.single_task<class test_kernel3>(f16);


This test is no longer necessary as it is the same as FuncObj above.

premanandrao · 2020-11-10T15:30:47Z

clang/include/clang/Basic/AttrDocs.td

+When applied to a lambda or function call operator (of a function object)
+on device, this requests, to the extent possible, that statically-scheduled
+clusters handle stalls using a stall-enable signal to freeze computation
+within the cluster.


Do we need to say that this is ignored on host, @Fznamznon?
If so, suggest, somethine like: 'This attribute is ignored on the host'.

I don't have a strong opinion here, other FPGA attributes are likely also ignored on host, but most of their docs don't mention anything about host. This doc also says "When applied to a lambda or function call operator (of a function object) on device " and nothing about host, which might be a clue that this attribute is intended for device use. However explicit statement that this attribute doesn't have any effect on host does seem good. So, I think we could add it, but it is not really something that should block this patch if we don't do it.

I think it is good for users to mention about this. I have updated doc.

Signed-off-by: Soumi Manna <[email protected]>

elizabethandrews

LGTM. Thanks!

clang/lib/Sema/SemaSYCL.cpp

smanna12 · 2020-11-10T17:11:32Z

Thank you Everyone for the reviews. @premanandrao, are you OK with the change?

premanandrao · 2020-11-10T17:22:02Z

Thank you Everyone for the reviews. @premanandrao, are you OK with the change?

Absolutely!

smanna12 · 2020-11-10T17:24:51Z

Thanks. @bader, Patch is ready for merge.

…ANY function [[intel::use_stall_enable_clusters]] is a function attribute, not a kernel one. It can be applied to any function or lambda. This patch removes the restriction that was added on PR: intel#2734 to allow propagating the attribute to kernel if it is applied to any function the kernel calls. Signed-off-by: Soumi Manna <[email protected]>

…any function (#3900) * [SYCL][FPGA] Apply [[intel::use_stall_enable_clusters]] attribute to ANY function [[intel::use_stall_enable_clusters]] is a function attribute, not a kernel one. It can be applied to any function or lambda. This patch removes the restriction that was added on PR: #2734 to allow propagating the attribute to kernel if it is applied to any function the kernel calls. Signed-off-by: Soumi Manna <[email protected]>

smanna12 added 3 commits November 5, 2020 06:20

[SYCL] update patches

7bcc2a0

Signed-off-by: Soumi Manna <[email protected]>

[SYCL] Fix Clang-format issues

9a6413d

Signed-off-by: Soumi Manna <[email protected]>

Fznamznon reviewed Nov 5, 2020

View reviewed changes

address review comment

e08afc1

Signed-off-by: Soumi Manna <[email protected]>

smanna12 mentioned this pull request Nov 6, 2020

Select device for FPGA specific attributes since intelfpga namespace was deprecated in favor of just intel namespace #2740

Open

smanna12 added 2 commits November 6, 2020 20:17

Update patches

08b4e78

Signed-off-by: Soumi Manna <[email protected]>

Fix Clang format issues

9198720

Signed-off-by: Soumi Manna <[email protected]>

smanna12 marked this pull request as ready for review November 8, 2020 22:08

smanna12 requested review from elizabethandrews and premanandrao as code owners November 8, 2020 22:08

smanna12 requested review from Fznamznon and MrSidims November 8, 2020 22:09

AlexeySotkin reviewed Nov 9, 2020

View reviewed changes

Address review comment

f8bea40

Signed-off-by: Soumi Manna <[email protected]>

smanna12 requested a review from AlexeySotkin November 9, 2020 14:27

elizabethandrews reviewed Nov 9, 2020

View reviewed changes

smanna12 added 2 commits November 9, 2020 09:31

Address review comments

4271118

Signed-off-by: Soumi Manna <[email protected]>

update doc

63eeda7

Signed-off-by: Soumi Manna <[email protected]>

smanna12 requested a review from elizabethandrews November 9, 2020 17:50

premanandrao reviewed Nov 9, 2020

View reviewed changes

Address review comments

6eb62f5

Signed-off-by: Soumi Manna <[email protected]>

smanna12 requested a review from premanandrao November 9, 2020 21:14

Fznamznon reviewed Nov 10, 2020

View reviewed changes

smanna12 added 2 commits November 10, 2020 06:50

Update patch and address review comments

3a648b8

Signed-off-by: Soumi Manna <[email protected]>

Fix Clang-fromat issues

af28ae1

Signed-off-by: Soumi Manna <[email protected]>

smanna12 requested a review from Fznamznon November 10, 2020 15:10

elizabethandrews reviewed Nov 10, 2020

View reviewed changes

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

Remove redundant test

d12bd0a

Signed-off-by: Soumi Manna <[email protected]>

premanandrao reviewed Nov 10, 2020

View reviewed changes

Update doc

f6ab893

Signed-off-by: Soumi Manna <[email protected]>

Fznamznon approved these changes Nov 10, 2020

View reviewed changes

elizabethandrews approved these changes Nov 10, 2020

View reviewed changes

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

premanandrao approved these changes Nov 10, 2020

View reviewed changes

bader merged commit 8fbf4bb into intel:sycl Nov 11, 2020

smanna12 mentioned this pull request Jun 8, 2021

[SYCL][FPGA] Apply [[intel::use_stall_enable_clusters]] attribute to any function #3900

Merged

	} else if (const auto *A = D->getAttr<SYCLIntelStallEnableAttr>()) {
	Diag(D->getLocation(), diag::err_opencl_kernel_attr) << A;
	D->setInvalidDecl();

		// CHECK: define spir_kernel void @"{{.}}test_kernel2"() #0 {{.}} !stall_enable [[FOO:![0-9]+]]
		// CHECK: define spir_kernel void @"{{.}}test_kernel3"() #0 {{.}} !stall_enable [[FOO:![0-9]+]]

[SYCL] Add support for new FPGA function attribute stall_enable #2734

[SYCL] Add support for new FPGA function attribute stall_enable #2734

Uh oh!

Conversation

smanna12 commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fznamznon 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smanna12 Nov 9, 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!

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!

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!

smanna12 Nov 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

elizabethandrews left a comment

smanna12 commented Nov 5, 2020 •

edited

Loading

smanna12 Nov 9, 2020 •

edited

Loading

smanna12 Nov 9, 2020 •

edited

Loading