Skip to content

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

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 8 commits into from
Sep 8, 2020
Merged

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

merged 8 commits into from
Sep 8, 2020

Conversation

schittir
Copy link
Contributor

@schittir schittir commented Jul 7, 2020 •
edited
Loading

  • Fix bug that propagates attributes from only canoncial declaration
  • Handle conflicting attributes
  • Add a test

Fznamznon
Fznamznon reviewed Jul 8, 2020
View reviewed changes
@schittir schittir marked this pull request as draft August 14, 2020 01:54
@schittir
[SYCL] Propagate attributes from redeclarations to SYCL kernel
3b06d7b 
- Fix bug that propagates attributes from only canoncial declaration
- Add a test
@schittir
Add conflicting attributes case to test
be9635e
@schittir
Handle another case of conflicting attributes for redeclarations
84e458e 
- throw error when same attributes with different parameters are
applied
- add test case
@schittir schittir force-pushed the propagate_redeclaration_attributes branch from d61e65e to 84e458e Compare September 3, 2020 01:16
@schittir schittir marked this pull request as ready for review September 3, 2020 18:11
@schittir
Handle another case of conflicting attributes from redeclarations
68befb9 
- address max_work_group_size attribute applied with different
parameters
@schittir schittir requested a review from Fznamznon September 4, 2020 07:16
@elizabethandrews
Copy link
Contributor

I don't think this lit failure is related to your patch. l've seen it before. @bader is this a known issue?

@bader
Copy link
Contributor

bader commented Sep 4, 2020

I haven't seen this failure before.

@elizabethandrews
Copy link
Contributor

Hmm....ok. I'm pretty sure I have. The error message "error in a[0]=7!=7" looks very familiar.

@schittir if you can't reproduce the issue locally, you can try re-building this test and then file a bug report if it is flaky.

@schittir
Copy link
Contributor Author

schittir commented Sep 4, 2020 •
edited
Loading

Hmm....ok. I'm pretty sure I have. The error message "error in a[0]=7!=7" looks very familiar.

@schittir if you can't reproduce the issue locally, you can try re-building this test and then file a bug report if it is flaky.

@elizabethandrews @bader
The error was unrelated to my patch. It disappeared with a rebuild.
Also all my local lit tests passed prior to commit.

@schittir
Copy link
Contributor Author

schittir commented Sep 4, 2020 •
edited
Loading

Hi @premanandrao could you please review this?
Just pinging in case it didn't catch your attention. Thank you!

premanandrao
premanandrao reviewed Sep 4, 2020
View reviewed changes
@schittir
Refactor repetetive code into a function
1bd38ad 
and change pattern matching for KernelName in lit test
Fznamznon
Fznamznon reviewed Sep 7, 2020
View reviewed changes
@schittir schittir requested a review from Fznamznon September 8, 2020 07:35
@schittir
Reversing the accidental change to the test
9f18cb6 
The previous commit had the parameters of the attributes of func3
changed unintentionally. This is inconsequential except for code-review
consistency purpose.
@schittir
Change test to use kernel_single_task from Inputs/sycl.hpp
8eba1d0 
Pass functions as lambda parameters instead of calling them from kernel
body
@bader bader merged commit 7ec85b0 into intel:sycl Sep 8, 2020
@smanna12 smanna12 mentioned this pull request Feb 23, 2022
Merged
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
@aarongreig
Merge pull request #2063 from nrspruit/refactor_l0_default_init
9ffb755 
[L0] Refactor to remove default constructor inits
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fznamznon Fznamznon Fznamznon approved these changes

@elizabethandrews elizabethandrews Awaiting requested review from elizabethandrews

@premanandrao premanandrao Awaiting requested review from premanandrao

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
@schittir @elizabethandrews @bader @Fznamznon @premanandrao