-
Notifications
You must be signed in to change notification settings - Fork 736
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
Deprecate intel reqd sub group size #15798
base: sycl
Are you sure you want to change the base?
Deprecate intel reqd sub group size #15798
Conversation
dklochkov-emb
commented
Oct 22, 2024
- [[intel::reqd_sub_group_size]] was made deprecated. Warning will be raised when it is used
- Test to check warning was added into separate file (tests/warnings_deprecated.cpp) because tests/warnings.cpp contains flag to ignore deprecated warnings
- Test to check modern attribute - [[sycl::reqd_sub_group_size]] - was added to check that no one warning was raised
- Changes to use modern [[sycl::reqd_sub_group_size]] in tests were done
- Some of the tests named sub-group-size got compiler flag to ignore new warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to sycl/test-e2e/AOT
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SYCLcompat tests changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
esimd/tools lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JM changes LGTM
@@ -2859,6 +2859,25 @@ static void handleWorkGroupSize(Sema &S, Decl *D, const ParsedAttr &AL) { | |||
WorkGroupAttr(S.Context, AL, WGSize[0], WGSize[1], WGSize[2])); | |||
} | |||
|
|||
// Handles reqd_sub_group_size and sub_group_size. | |||
static void handleSubGroupSize(Sema &S, Decl *D, const ParsedAttr &AL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecation should not require the addition of a new handler here. Are the semantic checks expected to be different for for both spellings? If not I would just use the old handler and modify checkDeprecatedSYCLAttributeSpelling
which I see you have already done.
@@ -32,20 +32,34 @@ int main() { | |||
}); | |||
return 0; | |||
} | |||
// expected-note@+2 {{did you mean to use 'sycl::reqd_sub_group_size' instead?}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to replace all uses of deprecated spelling with new spelling instead of adding additional warning checks. Please keep/add one test with deprecated spelling to ensure warning is emitted.
@@ -10,20 +10,26 @@ | |||
|
|||
#include "Inputs/sycl.hpp" | |||
|
|||
// expected-note@+4 {{did you mean to use 'sycl::reqd_sub_group_size' instead?}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments here.
@@ -1,4 +1,4 @@ | |||
// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -fsyntax-only -verify -pedantic %s | |||
// RUN: %clang_cc1 -fsycl-is-device -Wno-error=deprecated-declarations -internal-isystem %S/Inputs -fsyntax-only -verify -pedantic %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why this additional option was added when you do seem to be using the deprecated spelling in this test?