Skip to content

[SYCL] Add sycl-single-task implict property on single_task #8190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

steffenlarsen
Copy link
Contributor

This commit adds the "sycl-single-task" LLVM IR attribute to all SYCL kernels originating from single_task calls. Leveraging the existing mechanism for creating attributes for the SYCL kernel compile-time properties extension, applying the add_ir_attributes_function to SYCL kernel wrappers, this new attribute acts like a kernel compile-time property.

To avoid this implicit attribute from causing property/attribute conflict warnings, a special rule is made in the frontend to skip the conflict check if it is the only value in the add_ir_attributes_function attribute.

Additionally, this commit adds special handling of "sycl-single-task" when targeting "spir64_fpga", in which case it will cause a "max_global_work_dim" metadata node to be created with value "0" on functions where it is present.

This commit adds the "sycl-single-task" LLVM IR attribute to all
SYCL kernels originating from single_task calls. Leveraging the existing
mechanism for creating attributes for the SYCL kernel compile-time
properties extension, applying the add_ir_attributes_function to SYCL
kernel wrappers, this new attribute acts like a kernel compile-time
property.

To avoid this implicit attribute from causing property/attribute
conflict warnings, a special rule is made in the frontend to skip the
conflict check if it is the only value in the add_ir_attributes_function
attribute.

Additionally, this commit adds special handling of "sycl-single-task"
when targeting "spir64_fpga", in which case it will cause a
"max_global_work_dim" metadata node to be created with value "0" on
functions where it is present.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen requested a review from a team February 2, 2023 22:27
@steffenlarsen steffenlarsen requested review from a team as code owners February 2, 2023 22:27
@steffenlarsen steffenlarsen temporarily deployed to aws February 2, 2023 22:52 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws February 2, 2023 23:38 — with GitHub Actions Inactive
Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have a request for adding comments, but otherwise, FE changes look okay to me.

@@ -8007,10 +8007,23 @@ static bool checkSYCLAddIRAttributesMergeability(const AddIRAttrT &NewAttr,

void Sema::CheckSYCLAddIRAttributesFunctionAttrConflicts(Decl *D) {
const auto *AddIRFuncAttr = D->getAttr<SYCLAddIRAttributesFunctionAttr>();
if (!AddIRFuncAttr || AddIRFuncAttr->args_size() == 0 ||
if (!AddIRFuncAttr ||
hasDependentExpr(AddIRFuncAttr->args_begin(), AddIRFuncAttr->args_size()))
return;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a comment here about what this computation does?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good shout. I have added some comments to the skip criteria.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen temporarily deployed to aws February 8, 2023 11:08 — with GitHub Actions Inactive
Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for silly comments, I still don't fully understand the whole "comple time properties" thing.

if (NumArgsWithoutFilter == 0)
return;

// "sycl-single-task" is an implicitly used name which is present on all
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did I get it right that here under "implicit" actually meant "explicit" (i.e. written in code explicitly) but it happens in SYCL implementation code, so for an end user this is still "implicit"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! Implicit from the perspective of the user, but explicit inside the SYCL headers. I have clarified this in the comment.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen temporarily deployed to aws February 8, 2023 14:46 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws February 9, 2023 06:05 — with GitHub Actions Inactive
Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
@steffenlarsen steffenlarsen temporarily deployed to aws February 9, 2023 13:46 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws February 9, 2023 17:16 — with GitHub Actions Inactive
Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing approval till questions are clarified.

@steffenlarsen
Copy link
Contributor Author

@intel/llvm-reviewers-runtime & @intel/dpcpp-tools-reviewers - Friendly ping.

@steffenlarsen
Copy link
Contributor Author

@intel/llvm-reviewers-runtime - Friendly ping.

1 similar comment
@steffenlarsen
Copy link
Contributor Author

@intel/llvm-reviewers-runtime - Friendly ping.

@steffenlarsen steffenlarsen temporarily deployed to aws March 16, 2023 13:45 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen temporarily deployed to aws March 16, 2023 15:02 — with GitHub Actions Inactive
@steffenlarsen steffenlarsen merged commit 9170a5d into intel:sycl Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants