Skip to content

[SYCL] Allow [[sycl::work_group_size_hint]] to accept constant expr args #3785

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 25, 2021
Merged

[SYCL] Allow [[sycl::work_group_size_hint]] to accept constant expr args #3785

merged 7 commits into from
May 25, 2021

Conversation

AaronBallman
Copy link
Contributor

This changes the arguments from taking simple integer values to instead
accept arbitrary integer constant expression arguments. Additionally,
it implements the logic to correctly handle merging attributes on
redeclarations.

@AaronBallman
Allow [[sycl::work_group_size_hint]] to accept constant expr args
344a788 
This changes the arguments from taking simple integer values to instead
accept arbitrary integer constant expression arguments. Additionally,
it implements the logic to correctly handle merging attributes on
redeclarations.
@AaronBallman AaronBallman requested a review from smanna12 May 19, 2021 17:32
@AaronBallman
Copy link
Contributor Author

This is a follow-up to #3658, which added a test with a FIXME for this functionality. It does not attempt to make the attribute accept optional arguments, that is still outstanding work (covered by #3742).

premanandrao
premanandrao previously approved these changes May 19, 2021
View reviewed changes
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.

LGTM!

smanna12
smanna12 previously approved these changes May 19, 2021
View reviewed changes
Copy link
Contributor

@smanna12 smanna12 left a comment

Choose a reason for hiding this comment

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

LGTM.

@AaronBallman AaronBallman dismissed stale reviews from smanna12 and premanandrao via d410157 May 20, 2021 11:50
@AaronBallman
Copy link
Contributor Author

The clang-format failure seems like something is broken in CI: FileNotFoundError: [Errno 2] No such file or directory: 'clang-format-9'

@AaronBallman
Copy link
Contributor Author

The clang-format failure seems like something is broken in CI: FileNotFoundError: [Errno 2] No such file or directory: 'clang-format-9'

Pushing a merge commit to see if trigger CI again resolves the issue.

@AaronBallman
Copy link
Contributor Author

The clang-format failure seems like something is broken in CI: FileNotFoundError: [Errno 2] No such file or directory: 'clang-format-9'

Pushing a merge commit to see if trigger CI again resolves the issue.

Nope, same issue. Looks like CI is broken.

elizabethandrews
elizabethandrews previously approved these changes May 20, 2021
View reviewed changes
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.

LGTM!

@bader
Copy link
Contributor

bader commented May 20, 2021

The clang-format failure seems like something is broken in CI: FileNotFoundError: [Errno 2] No such file or directory: 'clang-format-9'

Pushing a merge commit to see if trigger CI again resolves the issue.

Could you try to merge sycl branch one more time, please? I pushed 7e4a386 and I hope it fixes the problem.

@AaronBallman
Copy link
Contributor Author

The clang-format failure seems like something is broken in CI: FileNotFoundError: [Errno 2] No such file or directory: 'clang-format-9'

Pushing a merge commit to see if trigger CI again resolves the issue.

Could you try to merge sycl branch one more time, please? I pushed 7e4a386 and I hope it fixes the problem.

Just pushed a merge commit, so let's see. Thanks for the speculative fix!

@AaronBallman
Copy link
Contributor Author

The clang-format failure seems like something is broken in CI: FileNotFoundError: [Errno 2] No such file or directory: 'clang-format-9'

Pushing a merge commit to see if trigger CI again resolves the issue.

Could you try to merge sycl branch one more time, please? I pushed 7e4a386 and I hope it fixes the problem.

Just pushed a merge commit, so let's see. Thanks for the speculative fix!

That seems to have fixed it, thanks @bader!

bader
bader reviewed May 20, 2021
View reviewed changes
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

Linux buildbots are offline and only one commit from this PR is not tested - d410157. I looked at this commit to check if it's safe to skip validation on Linux for this NFC patch and have a couple of comments regarding the refactoring.

premanandrao
premanandrao approved these changes May 24, 2021
View reviewed changes
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.

LGTM

@bader bader merged commit ef8e401 into intel:sycl May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader left review comments

@premanandrao premanandrao premanandrao approved these changes

@smanna12 smanna12 smanna12 approved these changes

@elizabethandrews elizabethandrews elizabethandrews left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AaronBallman @bader @premanandrao @elizabethandrews @smanna12