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

[SYCL] Remove support of deprecated {121, 1.2.1, sycl-1.2.1, 2017} values for the sycl-std= argument #14544

Merged
merged 4 commits into from
Jul 15, 2024

Conversation

AndreiZibrov
Copy link
Contributor

As a continuation of #14261 this PR makes impossible to call clang with -sycl-std=2017 option as well as with 121, 1.2.1, sycl-1.2.1 values. It also removes some tests which are available only with support of 2017/121

The rest of cleanup is to be continued with next submissions

Copy link
Contributor

@dm-vodopyanov dm-vodopyanov left a comment

Choose a reason for hiding this comment

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

Also need to remove/adjust:

@dm-vodopyanov
Copy link
Contributor

And I'm curious, how all these not removed tests are passing in pre-commit, if they still use removed values? Are we showing just some warning, and not an error, if user uses removed values for -sycl-std option?
Tagging @mdtoguchi

@AlexeySachkov
Copy link
Contributor

And I'm curious, how all these not removed tests are passing in pre-commit, if they still use removed values? Are we showing just some warning, and not an error, if user uses removed values for -sycl-std option? Tagging @mdtoguchi

SemaSYCL tests don't go through driver and therefore they most likely bypass argument validity check for the option. It then defaults to SYCL_None, but doesn't seem to cause any differences, i.e. they don't seem to test anything that is language-version-specific anymore.

@AndreiZibrov
Copy link
Contributor Author

AndreiZibrov commented Jul 12, 2024

many tests in clang/test/SemaSYCL/ still use removed values, as well as in in clang/test/Driver/ and clang/test/CodeGenSYCL/. Just grep for -sycl-std in the whole intel/llvm repo.

I'd be happy to know how many and which tests?

I can't find such tests

cd llvm
egrep -Irn "\-sycl\-std=((1\.2\.1)|(sycl\-1\.2\.1)|(121)|(2017))" . | egrep -v "^((\.git)|(\.vscode))" | wc -l
2

egrep -Irn "\-sycl\-std=((1\.2\.1)|(sycl\-1\.2\.1)|(121)|(2017)|(2020))" | egrep -v "^((\.git) | (\.vscode))" | wc -l
366

I'm not sure the old release notes from 13 months ago and from 4 ago have to be modified

egrep -Irn "\-sycl\-std=((1\.2\.1)|(sycl\-1\.2\.1)|(121)|(2017))" | egrep -v "^((\.git)|(\.vscode))"
sycl/ReleaseNotes.md:749:- Deprecated usage of `-sycl-std=1.2.1` [8dd6fdff]
sycl/ReleaseNotes.md:4269:    they should be trivially copyable. The `-sycl-std=1.2.1` driver option turns

thanks a lot for highlighted places:

@dm-vodopyanov

@AndreiZibrov AndreiZibrov requested a review from a team as a code owner July 12, 2024 14:17
@dm-vodopyanov
Copy link
Contributor

dm-vodopyanov commented Jul 12, 2024

I can't find such tests

Looks like I have outdated branch in which these tests are present.

I'm not sure the old release notes from 13 months ago and from 4 ago have to be modified

Yes, of course release notes should not be modified, they are not in my list above.

I cloned your repo to take a look closer: in the future, please don't create PR from sycl branch in your fork - create a branch with separate name in your fork.

@dm-vodopyanov dm-vodopyanov changed the title [SYCL] removing support of deprecated {121, 1.2.1, sycl-1.2.1, 2017} values for the sycl-std= argument [SYCL] Remove support of deprecated {121, 1.2.1, sycl-1.2.1, 2017} values for the sycl-std= argument Jul 12, 2024
@AndreiZibrov
Copy link
Contributor Author

I cloned your repo to take a look closer: in the future, please don't create PR from sycl branch in your fork - create a branch with separate name in your fork.

A misunderstanding occurred due to the simultaneous use of different branches😅. I temporarily set an incorrect branch as the default in my fork, which did not correspond to the branch used for the #14544.

As a result, an incorrect branch was utilized as default, leading to a discrepancy between the cloned files and the current state of the PR.

For precision, I would recommend using a specific commit ID or the PR ID as the target to be checked out as the end of the cloning process. This approach is detailed in the following resource: https://stackoverflow.com/questions/27567846/how-can-i-check-out-a-github-pull-request-with-git

I trust this clarifies the situation 😄

@AlexeySachkov AlexeySachkov merged commit c0c4d02 into intel:sycl Jul 15, 2024
14 checks passed
sarnex pushed a commit that referenced this pull request Aug 14, 2024
…ompat option specification (#14984)

As a continuation of [#14544](#14544)
this PR turns off attribute propagation specified by sycl-1.2.1. It also
removes rest of 2017/121 things like a definition of Sycl2017Compat
which had been turned off in
[#14239](#14239)
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.

5 participants