[SYCL] Propagate attributes from transitive calls to kernel #1878

schittir · 2020-06-12T07:25:09Z

Remove the (incorrect) distinction between direct and transitive calls for the
propagation of attributes to kernel
Handle conflicting attributes
Add a test showing propagation of attributes from transitive call to kernel
Correct failing lit tests to reflect new behavior

clang/lib/Sema/SemaSYCL.cpp

erichkeane · 2020-06-19T18:41:05Z

Is this right that you're only changing the comment now?

schittir · 2020-06-19T18:58:33Z

Is this right that you're only changing the comment now?

Yes. This is supposed to address the issue #1811
Also changing commit message to "[SYCL] Fixing comment about propagation of attributes to kernel".

Unless you think I should also include changing SYCLKernel->addAttr(A); part to contain cloned attributes to kernel, like in the previous commit f611c15
This could be kept separate.

schittir · 2020-06-19T19:48:44Z

Is this right that you're only changing the comment now?

Yes. This is supposed to address the issue #1811
Also changing commit message to "[SYCL] Fixing comment about propagation of attributes to kernel".

Unless you think I should also include changing SYCLKernel->addAttr(A); part to contain cloned attributes to kernel, like in the previous commit f611c15
This could be kept separate.

I could add a test case to show that indirect calls to kernel will be ignored with a warning

erichkeane · 2020-06-19T19:50:31Z

Is this right that you're only changing the comment now?

Yes. This is supposed to address the issue #1811
Also changing commit message to "[SYCL] Fixing comment about propagation of attributes to kernel".
Unless you think I should also include changing SYCLKernel->addAttr(A); part to contain cloned attributes to kernel, like in the previous commit f611c15
This could be kept separate.

I could add a test case to show that indirect calls to kernel will be ignored with a warning

That is perhaps a useful test. The addKernel cloning parts SHOULD happen, but is likely better in a separate patch.

schittir · 2020-06-20T00:11:43Z

Is this right that you're only changing the comment now?

Yes. This is supposed to address the issue #1811
Also changing commit message to "[SYCL] Fixing comment about propagation of attributes to kernel".
Unless you think I should also include changing SYCLKernel->addAttr(A); part to contain cloned attributes to kernel, like in the previous commit f611c15
This could be kept separate.

I could add a test case to show that indirect calls to kernel will be ignored with a warning

That is perhaps a useful test. The addKernel cloning parts SHOULD happen, but is likely better in a separate patch.

Agreed.

kbobrovs

@schittir, ##1811 provides a test case exposing the problem, but it is not a complete test case and can't be used as is. The essence of the problem is:

define dso_local spir_kernel void @_ZTSZ11invoke_foo2vE10KernelName() #0 !kernel_arg_addr_space !4 !kernel_arg_access_qual !4 !kernel_arg_type !4 !kernel_arg_base_type !4 !kernel_arg_type_qual !4 !no_global_work_offset !4 {
no_global_work_offset got propagated from unrelated function w/o warning.

Did you try it and made sure it does not happen? I'd expect to see a test case which ensures there is no no_global_work_offset on spir_kernel in the generated IR. I think for this test case to pass code changes are actually needed. In current implementation any function called from kernel is considered a kernel body function (functor or lambda), but there can be (and are) calls to other functions in the kernel - kernel calls things like item constructors before calling the kernel body function, and this is not distinguished.

Also I think before we fix this part, there needs to be some explanation/theoretical justification why attributes from functions that are not kernel body functions can't be propagated to kernels.

schittir · 2020-06-22T23:10:46Z

@kbobrovs

@schittir, ##1811 provides a test case exposing the problem, but it is not a complete test case and can't be used as is. The essence of the problem is:

define dso_local spir_kernel void @_ZTSZ11invoke_foo2vE10KernelName() #0 !kernel_arg_addr_space !4 !kernel_arg_access_qual !4 !kernel_arg_type !4 !kernel_arg_base_type !4 !kernel_arg_type_qual !4 !no_global_work_offset !4 {
no_global_work_offset got propagated from unrelated function w/o warning.
Did you try it and made sure it does not happen?

Dmitry pointed out that "the current compiler behavior is such that the left-sided (aka declaration attributes) shall be propagated to the called as well", which is consistent with spec1.2.1 #6.7 ; Therefore direct calls within kernel body will pass on the attributes to the kernel. The "unrelated function" highlighted in @kbobrovs example at #1811 is considered a direct call because it is being called within the kernel body, and therefore propagating it to the kernel would be considered correct. @erichkeane would you agree?
My latest patch 0a76fe6 I includes a test case which shows that a call that is not direct is indeed ignored with a warning.

In current implementation any function called from kernel is considered a kernel body function (functor or lambda), but there can be (and are) calls to other functions in the kernel - kernel calls things like item constructors before calling the kernel body function, and this is not distinguished.

I will follow up on these offline.

Also I think before we fix this part, there needs to be some explanation/theoretical justification why attributes from functions that are not kernel body functions can't be propagated to kernels.

Yes, I would like to understand this too.

kbobrovs · 2020-06-23T07:38:12Z

@schittir - spec1.2.1 6.7 is the "theoretical justification" I was looking for - thanks.
But the spec does not distinguish direct vs transitive calls - propagation is done from the leaves of the call graph up, if I read it correctly:

In SYCL, these attributes are legal on device functions and their specification is propagated down > to any caller of those device functions,

So I still don't see what this is based on in the spec:

// Allow the following kernel attributes only on lambdas, functions and
// function objects that are called directly from a kernel.
// For all other cases, emit a warning and ignore.

So if implementation follows the above comment, it seems to be incorrect.

Also note that the spec about 3 attributes: vec_type_hint, work_group_size_hint and reqd_work_group_size and my testcase uses no_global_work_offset not mentioned in the spec. The comment with justification should mention that some other attributes not mentioned in the spec are treated likewise, because we believe it is the right thing.

@Ruyk, @Naghasan - could you please help interpret section "6.7 Attributes" of the SYCL spec?

kbobrovs · 2020-06-26T17:13:23Z

According to @Naghasan clarification, this is the correct interpretation:

the spec does not distinguish direct vs transitive calls - propagation is done from the leaves of the call graph up

Also other relevant attributes like no_global_work_offset should be treated likewise. So the approach described in the comment is incorrect.

schittir · 2020-06-26T19:59:17Z

Thank you for the clarification.
@kbobrovs
In that case, what needs to be done here is the removal of the behaviour that causes the following test to pass, correct? 0a76fe6#diff-4c6b21a8286f99399550da1b451b0b70
Couldn't find an official definition for a "transitive" call - assuming that it is the same as not_direct() call w.r.t __my_kernel__

This should make no difference to the behaviour of the test described here #1811 i.e., the propagation of attribute to kernel without a warning is accepted as it is.

Also, is the spec clarification from @Naghasan recorded somewhere you can share?

clang/test/SemaSYCL/check-indirect-attribute-propagation.cpp

clang/lib/Sema/SemaSYCL.cpp

AlexeySachkov · 2020-07-10T17:25:23Z

Also other relevant attributes like no_global_work_offset should be treated likewise. So the approach described in the comment is incorrect.

Tagging @MrSidims here. Probably it's time to write proper specs for FPGA-specific attributes and define whether they should be "left-sided" with propagation enabled or "right-sided" with propagation disabled. Please note that current spelling and propagation mechanism is deprecated in SYCL 2020 Provisional spec

elizabethandrews · 2020-07-13T15:12:26Z

clang/test/SemaSYCL/check-notdirect-attribute-propagation.cpp

@@ -16,5 +16,8 @@ void parallel_for(Type lambda) {
 }

 void invoke_foo2() {
+  // CHECK-LABEL:  FunctionDecl {{.*}} invoke_foo2 'void ()'
+  // CHECK:  `-FunctionDecl {{.*}} _ZTSZ11invoke_foo2vE10KernelName 'void ()'


I would suggest {{.}}KernelName{{.}} Differences in name mangling on Windows will cause issues otherwise.

I agree with this thought, and changed it per your suggestion. However, please note that there are other lit tests following this name mangled pattern, with which I intended to be consistent.

clang/test/SemaSYCL/check-indirect-attribute-propagation.cpp

clang/test/SemaSYCL/check-notdirect-attribute-propagation.cpp

schittir · 2020-07-27T22:18:29Z

Accidentally pushed 6 unrelated commits to this pull request. Please ignore them. Working on fix.

Also adding a test to check that the kernel does not get attributes from functions that are not directly called by it.

- Remove the (incorrect) distinction between direct and transitive calls for the propagation of attributes to kernel - Add a test showing propagation of attributes from transitive call to kernel

premanandrao

New changes appear to be minor. LGTM

…rogram * upstream/sycl: (609 commits) [SYCL] Fix fail in the post commit testing (intel#2210) [SYCL] Materialize shadow local variables for byval arguments before use (intel#2200) [SYCL] Support lambda functions passed to reduction (intel#2190) [SYCL][USM] Improve USM Allocator. (intel#2026) [SYCL] Disallow mutable lambdas (intel#1785) [SYCL][ESIMD] Setup compilation pipeline for ESIMD (intel#2134) [SYCL] Fix not found kernel due to empty kernel name when using set_arg(s) (intel#2181) [SYCL] Fixed check for set_arg (intel#2203) Refactor indirect access calls to minimize invocations. (intel#2185) [SYCL][NFC] Fix potential null-pointer access (intel#2197) [SYCL] Propagate attributes from transitive calls to kernel (intel#1878) [SYCL] Fix warnings from static analysis tool (intel#2193) [SYCL][NFC] Fix ac_float test for compilation with FE optimizations (intel#2184) [GitHub Actions] Uplift clang-format version to 10 (intel#2194) [SYCL][ESIMD] Pass to replace simd* parameters with native llvm vectors. (intel#2097) [SYCL][NFC] Fixed SYCL_PI_TRACE output while selecting a device. (intel#2192) [SYCL][FPGA] New spec for controlling load-store units in FPGAs (intel#2158) [SYCL][Doc] Clarify reqd_sub_group_size (intel#2103) [SYCL] Remove noreturn function attribute (intel#2165) [SYCL] Aligned set_arg behaviour with SYCL specification (intel#2159) ...

We removed the following restrictions on intel#1878 // Allow the following kernel attributes only on lambda functions and // function objects that are called directly from a kernel (i.e. the one // passed to the parallel_for function). For all other cases, // emit a warning and ignore. 1. SYCLIntelKernelArgsRestrictAttr 2. SYCLIntelNumSimdWorkItemsAttr 3. SYCLIntelMaxWorkGroupSizeAttr 4. SYCLIntelMaxGlobalWorkDimAttr This patch updates the attribute document to reflect the changes since we do not ignore the kernel attributes anymore if it is applied to a function called from a device kernel and the attribute gets propagated to a kernel. Signed-off-by: Soumi Manna <[email protected]>

We removed the following restrictions on #1878 // Allow the following kernel attributes only on lambda functions and // function objects that are called directly from a kernel (i.e. the one // passed to the parallel_for function). For all other cases, // emit a warning and ignore. 1. SYCLIntelKernelArgsRestrictAttr 2. SYCLIntelNumSimdWorkItemsAttr 3. SYCLIntelMaxWorkGroupSizeAttr 4. SYCLIntelMaxGlobalWorkDimAttr This patch updates the attribute document to reflect the changes since we do not ignore the kernel attributes anymore if it is applied to a function called from a device kernel and the attribute gets propagated to a kernel. Signed-off-by: Soumi Manna <[email protected]>

This entity represents a module in the programming language, for example a Fortran module. Spec: KhronosGroup/SPIRV-Registry#186 The implementation is the same as for SPV_INTEL_debug_module extension. Spec for extension: https://github.com/intel/llvm/blob/sycl/sycl/doc/design/spirv-extensions/SPV_INTEL_debug_module.asciidoc Original commit: KhronosGroup/SPIRV-LLVM-Translator@6a0368f

schittir requested review from elizabethandrews, Fznamznon and premanandrao as code owners June 12, 2020 07:25

schittir marked this pull request as draft June 12, 2020 07:35

erichkeane reviewed Jun 12, 2020

View reviewed changes

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

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

schittir force-pushed the kernel_attributes_propagation branch from f611c15 to b28af10 Compare June 19, 2020 17:29

schittir requested a review from erichkeane June 19, 2020 18:37

schittir marked this pull request as ready for review June 19, 2020 18:59

erichkeane previously approved these changes Jun 22, 2020

View reviewed changes

Fznamznon requested a review from kbobrovs June 22, 2020 14:17

kbobrovs suggested changes Jun 22, 2020

View reviewed changes

schittir dismissed erichkeane’s stale review via 4d90d1c July 10, 2020 04:48

elizabethandrews reviewed Jul 10, 2020

View reviewed changes

clang/test/SemaSYCL/check-indirect-attribute-propagation.cpp Outdated Show resolved Hide resolved

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

schittir changed the title ~~[SYCL] Propagate to kernel only the attributes of its lambda or function object~~ [SYCL] Propagate attributes from transitive calls to kernel Jul 10, 2020

MrSidims self-requested a review July 10, 2020 21:10

elizabethandrews reviewed Jul 13, 2020

View reviewed changes

clang/test/SemaSYCL/check-indirect-attribute-propagation.cpp Outdated Show resolved Hide resolved

clang/test/SemaSYCL/check-notdirect-attribute-propagation.cpp Outdated Show resolved Hide resolved

schittir force-pushed the kernel_attributes_propagation branch from e0234b8 to dd9e2d0 Compare July 13, 2020 20:01

schittir requested review from elizabethandrews and kbobrovs July 13, 2020 20:04

schittir requested review from AlexeySachkov, Pennycook and a team as code owners July 27, 2020 21:43

schittir force-pushed the kernel_attributes_propagation branch from e3e52eb to 8a9164d Compare July 27, 2020 22:50

schittir added 11 commits July 27, 2020 15:52

[SYCL] Fixing comment about propagation of attributes to kernel

fd4591c

[SYCL] Fixing comment about propagation of attributes to kernel

6b5d4a9

[SYCL] Fixing comment about propagation of attributes to kernel

3857f54

Also adding a test to check that the kernel does not get attributes from functions that are not directly called by it.

Propagate attributes from transitive calls to kernel

ef2deea

- Remove the (incorrect) distinction between direct and transitive calls for the propagation of attributes to kernel - Add a test showing propagation of attributes from transitive call to kernel

Correct failing lit tests

3426ef0

Minor changes to address comments and resolve branch merge conflicts

acb7a65

Fix formatting issues reported by clang-format-check bot

eb3a32e

Add diagnostics for conflicting attributes from transitive calls

49c747c

Fix formatting

e3e79b1

Fix lit test failure caused by newer commits to repo

882b3d0

Fix the bug causing lit test failure

1735df4

schittir removed request for a team, Pennycook and againull July 27, 2020 23:04

schittir force-pushed the kernel_attributes_propagation branch from 8a9164d to 1735df4 Compare July 27, 2020 23:05

schittir requested a review from premanandrao July 27, 2020 23:19

premanandrao approved these changes Jul 28, 2020

View reviewed changes

bader merged commit 5c91609 into sycl Jul 29, 2020

bader deleted the kernel_attributes_propagation branch July 29, 2020 07:30

schittir mentioned this pull request Sep 3, 2020

[SYCL] Propagate attributes from redeclarations to SYCL kernel #2063

Merged

smanna12 mentioned this pull request Jan 26, 2021

[SYCL][NFC] Update Attribute Document #3102

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Propagate attributes from transitive calls to kernel #1878

[SYCL] Propagate attributes from transitive calls to kernel #1878

schittir commented Jun 12, 2020 •

edited

Loading

erichkeane commented Jun 19, 2020

schittir commented Jun 19, 2020

schittir commented Jun 19, 2020

erichkeane commented Jun 19, 2020

schittir commented Jun 20, 2020

kbobrovs left a comment

schittir commented Jun 22, 2020 •

edited

Loading

kbobrovs commented Jun 23, 2020

kbobrovs commented Jun 26, 2020

schittir commented Jun 26, 2020 •

edited

Loading

AlexeySachkov commented Jul 10, 2020

elizabethandrews Jul 13, 2020

schittir Jul 13, 2020

schittir commented Jul 27, 2020

premanandrao left a comment

[SYCL] Propagate attributes from transitive calls to kernel #1878

[SYCL] Propagate attributes from transitive calls to kernel #1878

Conversation

schittir commented Jun 12, 2020 • edited Loading

erichkeane commented Jun 19, 2020

schittir commented Jun 19, 2020

schittir commented Jun 19, 2020

erichkeane commented Jun 19, 2020

schittir commented Jun 20, 2020

kbobrovs left a comment

Choose a reason for hiding this comment

schittir commented Jun 22, 2020 • edited Loading

kbobrovs commented Jun 23, 2020

kbobrovs commented Jun 26, 2020

schittir commented Jun 26, 2020 • edited Loading

AlexeySachkov commented Jul 10, 2020

elizabethandrews Jul 13, 2020

Choose a reason for hiding this comment

schittir Jul 13, 2020

Choose a reason for hiding this comment

schittir commented Jul 27, 2020

premanandrao left a comment

Choose a reason for hiding this comment

schittir commented Jun 12, 2020 •

edited

Loading

schittir commented Jun 22, 2020 •

edited

Loading

schittir commented Jun 26, 2020 •

edited

Loading