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.

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

[Driver][clang-offload-bundler] Add asserts requirement for test relying on debug build #10379

Open
wants to merge 7 commits into
base: sycl
Choose a base branch
from

Conversation

asudarsa
Copy link
Contributor

This PR addresses #10238

Thanks

@asudarsa asudarsa requested a review from a team as a code owner July 14, 2023 03:49
@asudarsa asudarsa temporarily deployed to aws July 14, 2023 04:06 — with GitHub Actions Inactive
@asudarsa asudarsa temporarily deployed to aws July 14, 2023 04:45 — with GitHub Actions Inactive
@asudarsa asudarsa requested review from a team and bader as code owners July 20, 2023 14:46
@asudarsa asudarsa requested a review from sergey-semenov July 20, 2023 14:46
@asudarsa asudarsa removed request for a team July 20, 2023 14:52
@asudarsa asudarsa removed the request for review from sergey-semenov July 20, 2023 14:54
@asudarsa
Copy link
Contributor Author

Sorry. some reviewers got added due to an intermediate 'buggy' merge.

Thanks

@asudarsa asudarsa temporarily deployed to aws July 20, 2023 15:30 — with GitHub Actions Inactive
@asudarsa asudarsa temporarily deployed to aws July 20, 2023 16:08 — with GitHub Actions Inactive
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.

I suggest we reset this file to the original state from main branch - https://github.com/intel/llvm/blob/main/clang/test/Driver/clang-offload-bundler-standardize.c.
Otherwise, there will be conflicts with the community changes.
I think there should be no SYCL specific changes in this file. Am I right?

@asudarsa
Copy link
Contributor Author

I suggest we reset this file to the original state from main branch - https://github.com/intel/llvm/blob/main/clang/test/Driver/clang-offload-bundler-standardize.c. Otherwise, there will be conflicts with the community changes. I think there should be no SYCL specific changes in this file. Am I right?

Ah.. I did not follow that we had a dedicated 'main' branch. I thought 'sycl' was our 'main' branch.
I can reset this file to be in sync with the 'main' copy.
Just curious, do we have to always try to be in sync with 'main' branch when possible?

Thanks

@bader
Copy link
Contributor

bader commented Jul 31, 2023

Just curious, do we have to always try to be in sync with 'main' branch when possible?

Yes. main is a mirror of https://github.com/llvm/llvm-project/tree/main branch and we need to the difference between main and sycl branches as minimal as possible to avoid conflicts during pulldown process. We can do this by upstreaming SYCL patches to the https://github.com/llvm/llvm-project/tree/main and avoid unnecessary changes in the sycl branch.

@asudarsa
Copy link
Contributor Author

asudarsa commented Aug 1, 2023

Just curious, do we have to always try to be in sync with 'main' branch when possible?

Yes. main is a mirror of https://github.com/llvm/llvm-project/tree/main branch and we need to the difference between main and sycl branches as minimal as possible to avoid conflicts during pulldown process. We can do this by upstreaming SYCL patches to the https://github.com/llvm/llvm-project/tree/main and avoid unnecessary changes in the sycl branch.

Thanks for the clarification @bader.
In this case, the only difference is the main branch has '// REQUIRES: asserts' added twice. May be the main branch needs to be fixed?

Thanks

@bader
Copy link
Contributor

bader commented Aug 1, 2023

Just curious, do we have to always try to be in sync with 'main' branch when possible?

Yes. main is a mirror of https://github.com/llvm/llvm-project/tree/main branch and we need to the difference between main and sycl branches as minimal as possible to avoid conflicts during pulldown process. We can do this by upstreaming SYCL patches to the https://github.com/llvm/llvm-project/tree/main and avoid unnecessary changes in the sycl branch.

Thanks for the clarification @bader. In this case, the only difference is the main branch has '// REQUIRES: asserts' added twice. May be the main branch needs to be fixed?

Thanks

That's an option as well. The final goal is the have the same version of the file in both branches.

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.

@asudarsa, please, resolve merge conflicts.

@maarquitos14
Copy link
Contributor

I think this PR can be closed: the changes made here seem to be already made in the current status of the file. Please, @asudarsa close if you agree.

Copy link
Contributor

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

@github-actions github-actions bot added the Stale label Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure in Clang :: Driver/clang-offload-bundler-standardize.c in post-commit
4 participants