-
Notifications
You must be signed in to change notification settings - Fork 792
[SYCL] Implement sycl-post-link tool #695
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] Implement sycl-post-link tool #695
Conversation
This is part of device code splitting feature. Please refer #631 for full design doc. |
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.
That looks good.
Just a few stylistic remarks since you are starting a new tool: this is a good opportunity to enter the 21th century. :-)
llvm/tools/sycl-split/sycl-split.cpp
Outdated
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.
All these thousands of (
and )
should be {
and }
in a new software like this one. :-)
llvm/tools/sycl-split/sycl-split.cpp
Outdated
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.
Yes! Let's be modern in 2019 and make Chandler happy! :-)
llvm/tools/sycl-split/sycl-split.cpp
Outdated
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.
Twine { }
llvm/tools/sycl-split/sycl-split.cpp
Outdated
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.
Twine { }
I have a general comment. It turns out we may soon need other kinds post-link processing on the device code. So I suggest to treat this tool as a placeholder for all such post-link LLVM passes so that we don't create a new tool each time. More concretely, rename it to something like sycl-post-link/sycl-post-link.cpp |
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.
Are these attributes required for the test to work? If not, suggest removing.
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.
They are not, removed.
4d4d339
to
253c73c
Compare
Per our agreement with Mariya, I'll be driving this PR for now.
Done. @keryell thanks for the comments. Applied most of them, the only part I disagree on is replacing parentheses with uniform initialization for constructors of temporaries. From LLVM coding standards (Do not use Braced Initializer Lists to Call a Constructor):
|
Interestingly, LLVM is doing the opposite of the C++ Core Guidelines https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-list The problem I have with using parenthesis with constructors is that it is less obvious that there is some object construction when browsing the code... |
ping. |
The tool will split single big linked device module into smaller ones using 'module-id' attribute applied to kernels. Signed-off-by: Mariya Podchishchaeva <[email protected]> Signed-off-by: Sergey Semenov <[email protected]>
Signed-off-by: Sergey Semenov <[email protected]>
Signed-off-by: Mariya Podchishchaeva <[email protected]>
253c73c
to
2c832f8
Compare
Signed-off-by: Mariya Podchishchaeva <[email protected]>
Signed-off-by: Mariya Podchishchaeva <[email protected]>
The tool will split single big linked device module into smaller ones
using 'module-id' attribute applied to kernels.
Signed-off-by: Mariya Podchishchaeva [email protected]