-
Notifications
You must be signed in to change notification settings - Fork 772
[SYCL] Add indeterminate constructor to work group memory interface #16003
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
Merged
martygrant
merged 25 commits into
intel:sycl
from
lbushi25:indeterminate_constructor_work_group_memory
Nov 15, 2024
Merged
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
e574aa7
Add indeterminate constructor to work group memory interface
lbushi25 8164ff2
Update tests to use indeterminate constructor instead of default one
lbushi25 543aa6f
Add indeterminate constructor to work group memory interface
lbushi25 a34b463
Fix typo
lbushi25 65a8e23
Fix compiler error
lbushi25 aac82e2
Formatting changes
lbushi25 d6709ed
Fix unused variable warning
lbushi25 847163d
Modify handling of SYCL special types to account for default construc…
lbushi25 f551ba4
Modify handling of SYCL special types to account for default construc…
lbushi25 172d717
Modify handling of SYCL special types to account for default construc…
lbushi25 e531b05
Make default constructor private
lbushi25 52bf8a4
Formatting changes
lbushi25 32c5cb6
Change naming convention to match rest of the code
lbushi25 418365d
Merge branch 'intel:sycl' into indeterminate_constructor_work_group_m…
lbushi25 eb31ca0
Use std::find_if instead of std::for_each to find default constructor
lbushi25 54abcd0
Revert back changes to default constructor
lbushi25 21c27fa
Fix formatting errors
lbushi25 8c94bfc
Revert "Fix formatting errors"
lbushi25 d5c79ec
Revert "Revert "Fix formatting errors""
lbushi25 0e3ae8a
Revert "Use std::find_if instead of std::for_each to find default con…
lbushi25 be386bc
Revert "Revert back changes to default constructor"
lbushi25 17639ae
Bypass default constructor access specifier in frontend and add tests…
lbushi25 6cc99d3
Add TODO to revisit the handling of special types
lbushi25 90bceb0
Formatting changes
lbushi25 3ea9103
Fix regex in filecheck pattern
lbushi25 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
// RUN: %clang_cc1 -fsycl-is-device -internal-isystem %S/Inputs -ast-dump -sycl-std=2020 %s | FileCheck %s | ||
|
||
// Check that AST is correctly generated for kernel arguments that inherit from work group memory. | ||
|
||
#include "sycl.hpp" | ||
|
||
sycl::queue myQueue; | ||
|
||
struct WorkGroupMemoryDerived : | ||
sycl::work_group_memory<int> { | ||
}; | ||
|
||
int main() { | ||
myQueue.submit([&](sycl::handler &h) { | ||
WorkGroupMemoryDerived DerivedObject{ h }; | ||
h.parallel_for<class kernel>([=] { | ||
DerivedObject.use(); | ||
}); | ||
}); | ||
return 0; | ||
} | ||
|
||
// CHECK: FunctionDecl {{.*}}kernel{{.*}} 'void (__local int *)' | ||
// CHECK-NEXT: ParmVarDecl {{.*}}used _arg__base '__local int *' | ||
// CHECK-NEXT: CompoundStmt {{.*}} | ||
// CHECK-NEXT: DeclStmt {{.*}} | ||
// CHECK-NEXT: VarDecl {{.*}} used __SYCLKernel {{.*}} cinit | ||
// CHECK-NEXT: InitListExpr {{.*}} | ||
// CHECK-NEXT: InitListExpr {{.*}} 'WorkGroupMemoryDerived' | ||
// CHECK-NEXT: CXXConstructExpr {{.*}} 'sycl::work_group_memory<int>' 'void () noexcept' | ||
// CHECK-NEXT: CXXMemberCallExpr {{.*}} 'void' | ||
// CHECK-NEXT: MemberExpr {{.*}} 'void (__local int *)' lvalue .__init {{.*}} | ||
// CHECK-NEXT: MemberExpr {{.*}} 'WorkGroupMemoryDerived' lvalue .DerivedObject | ||
// CHECK-NEXT: DeclRefExpr {{.*}} lvalue Var {{.*}} '__SYCLKernel' | ||
// CHECK-NEXT: ImplicitCastExpr {{.*}} '__local int *' <LValueToRValue> | ||
// CHECK-NEXT: DeclRefExpr {{.*}} '__local int *' lvalue ParmVar {{.*}} '_arg__base' '__local int *' | ||
// CHECK-NEXT: CompoundStmt {{.*}} | ||
// CHECK-NEXT: CXXOperatorCallExpr {{.*}} 'void' '()' | ||
// CHECK-NEXT: ImplicitCastExpr {{.*}} 'auto (*)() const -> void' <FunctionToPointerDecay> | ||
// CHECK-NEXT: DeclRefExpr {{.*}}'auto () const -> void' lvalue CXXMethod {{.*}} 'operator()' 'auto () const -> void' | ||
// CHECK-NEXT: ImplicitCastExpr {{.*}} | ||
// CHECK-NEXT: DeclRefExpr {{.*}}lvalue Var {{.*}} '__SYCLKernel' | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Does it actually complain during kernel construction if you don't set access?
If yes, does it complain when it should after this change?
Uh oh!
There was an error while loading. Please reload this page.
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.
During the construction of the SYCL Kernel Body, it complains that the constructor has an incorrect number of arguments because it doesn't see a public default constructor. After this change, it doesn't complain anymore during kernel construction but it does complain when using the default constructor in any other place for example if the user uses the default constructor, they will get a compilation error because its not public to them. Its also an approach that works for any special type so we don't have to change our handling based on the special type.
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.
This line in particular that you have reviewed sets the access back to its original value so that we don't accidentally leave the default constructor visible when it shouldn't be. Frankly, even without this line it seemed like the user could still not call the default constructor if it was defined private, but I chose to err on the safe side.
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.
I see. I suppose I would also prefer the safe side.
Shouldn't you be doing the same for each variant of
handleSpecialType
?Uh oh!
There was an error while loading. Please reload this page.
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.
Good question. This change is only relevant for the semantic action of the
SyclKernelBodyCreator
class which has 3 members namedhandleSpecialType
. One is for kernel handler which has its own thing going on so I don't think it needs these changes, the other is the one I've changed here and the remaining one is the one taking aCXXBaseSpecifier
. I suppose I should add it to this one too but I forgot, is that what you were asking about?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.
Right. Passing precommit also suggests lack of corresponding tests. I suppose a test should contain work group memory as a base to exercise the code.
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.
Ok. I will add such a test.
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.
I do not have another solution but this just feels very hacky and I really doubt community clang will accept this. This is out of the scope of the PR but the design of how we handle initialization of special types probably needs to change if we cannot use a default constructor. @tahonermann please weigh in here.