Skip to content
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

[DeviceAsan][NFC] Code Restructure #15843

Merged
merged 13 commits into from
Nov 22, 2024
Merged

[DeviceAsan][NFC] Code Restructure #15843

merged 13 commits into from
Nov 22, 2024

Conversation

AllanZyne
Copy link
Contributor

@AllanZyne AllanZyne commented Oct 24, 2024 •
edited
Loading

UR: oneapi-src/unified-runtime#2232

Code refactoring for implementing MemorySanitizer

@AllanZyne AllanZyne requested review from a team as code owners October 24, 2024 02:35
@AllanZyne AllanZyne requested a review from a team as a code owner October 24, 2024 02:42
@AllanZyne AllanZyne mentioned this pull request Oct 24, 2024
Merged
@AllanZyne AllanZyne marked this pull request as draft October 24, 2024 07:06
@AllanZyne AllanZyne marked this pull request as ready for review November 20, 2024 08:47
zhaomaosu
zhaomaosu approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@zhaomaosu zhaomaosu left a comment

Choose a reason for hiding this comment

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

lgtm

@AllanZyne
Copy link
Contributor Author

Hi reviewers, could you please prioritize the review of this PR?
This PR has no functional change, and is essential for our further development!
Thank you very much!

@kbenzie
Copy link
Contributor

kbenzie commented Nov 20, 2024

There are failures in the Jenkins/Precommit job which will need to be fixed.

@AllanZyne
Copy link
Contributor Author

Hi @intel/llvm-reviewers-runtime @intel/unified-runtime-reviewers, please review.
Thank you very much!

@aelovikov-intel
Copy link
Contributor

Hi @intel/llvm-reviewers-runtime ..., please review. Thank you very much!

What do you need us for? They only change under sycl/ is changing repo/commit in CMake files that you'd need to restore before merging. What am I missing?

@AllanZyne
Copy link
Contributor Author

AllanZyne commented Nov 21, 2024 •
edited
Loading

Hi @intel/llvm-reviewers-runtime ..., please review. Thank you very much!

What do you need us for? They only change under sycl/ is changing repo/commit in CMake files that you'd need to restore before merging. What am I missing?

Okay. Hi @pbalcer, I think I have got llvm-runtime-reviewers' approve.

@kbenzie
Copy link
Contributor

kbenzie commented Nov 21, 2024

This is still showing as a required code owner approval in the GitHub UI

screenshot

@AllanZyne
Copy link
Contributor Author

Hi @aelovikov-intel @intel/llvm-reviewers-runtime, could you please approve this PR? Thanks!

@AllanZyne
Copy link
Contributor Author

AllanZyne commented Nov 22, 2024 •
edited
Loading

Hi @intel/llvm-reviewers-runtime ..., please review. Thank you very much!

What do you need us for? They only change under sycl/ is changing repo/commit in CMake files that you'd need to restore before merging. What am I missing?

Hi, libdevice/cmake/modules/SYCLLibdevice.cmake is also owned by @intel/llvm-reviewers-runtime.

@AllanZyne
Copy link
Contributor Author

Hi @intel/llvm-gatekeepers, before I get approve, could you please merge this PR before other sanitizer related PRs?
I've synced with other members.
Thank you very much!

@AllanZyne AllanZyne mentioned this pull request Nov 22, 2024
Merged
@AllanZyne AllanZyne requested a review from a team as a code owner November 22, 2024 01:51
@AllanZyne
Copy link
Contributor Author

Hi @intel/dpcpp-devops-reviewers, could you please review this PR ASAP? Thanks!

steffenlarsen
steffenlarsen approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

LGTM!

@pbalcer
Copy link
Contributor

pbalcer commented Nov 22, 2024 

  SYCL :: Graph/Update/dyn_cgf_different_arg_nums.cpp

This failure is unrelated, and has a fix here: #16157

@intel/llvm-gatekeepers can this be merged with that unrelated fail in CI? It's blocking UR updates.

@steffenlarsen
Copy link
Contributor

Looks like it fails for other PRs too, so let's go ahead with it.

@steffenlarsen steffenlarsen merged commit 1e1ffc0 into sycl Nov 22, 2024
12 of 13 checks passed
@EwanC EwanC mentioned this pull request Nov 22, 2024
Merged
@AllanZyne AllanZyne deleted the review/yang/restructure_asan branch November 23, 2024 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steffenlarsen steffenlarsen steffenlarsen approved these changes

@mdtoguchi mdtoguchi mdtoguchi approved these changes

@zhaomaosu zhaomaosu zhaomaosu approved these changes

@pbalcer pbalcer pbalcer approved these changes

@cperkinsintel cperkinsintel Awaiting requested review from cperkinsintel cperkinsintel is a code owner automatically assigned from intel/llvm-reviewers-runtime

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

Successfully merging this pull request may close these issues.
7 participants
@AllanZyne @kbenzie @aelovikov-intel @pbalcer @steffenlarsen @zhaomaosu @mdtoguchi