-
Notifications
You must be signed in to change notification settings - Fork 724
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] migrate sycl-module-split to llvm-split #14559
base: sycl
Are you sure you want to change the base?
Conversation
Removes sycl-module-split tool and replaces all usages with llvm-split tool.
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.
LGTM, we should be able to integrate a lot deeper once we are using the spir-v backend so we can overload TargetMachine's splitModule
, but since we don't have that, this looks good!
@@ -6,7 +6,7 @@ | |||
; RUN: FileCheck %s -input-file=%t_0.sym --check-prefix CHECK-PER-SOURCE-SYM0 | |||
; RUN: FileCheck %s -input-file=%t_1.sym --check-prefix CHECK-PER-SOURCE-SYM1 | |||
; | |||
; RUN: sycl-module-split -split=source -S < %s -o %t1 | |||
; RUN: llvm-split -sycl-split=source -S < %s -o %t1 |
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.
sycl-post-link tests should not call sycl-module-split
or llvm-split
tools. This is very confusing.
@maksimsab, please, file an issue to refactor these tests.
Also, it's not clear why do we test pass functionality twice using different executables. Can we run just one tool to validate split logic?
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.
Migration of functionality of sycl-post-link
to library hasn't been completed yet. For example, ESIMD splitting is going to be moved to the library during this sprint.
During the migration we keep testing of sycl-post-link
since it is used in the default pipeline and we keep the testing of the library to be sure that there are no bugs.
In the end of the migration the library will be invoked from clang-linker-wrapper
and sycl-post-link
will be removed and these tests will move to the right directory. These tests weren't duplicated because it is easy to make a mistake when someone modifies one test and they forget to modify the equivalent duplicated test.
Proposed testing approach with llvm-split
is the same as one that was used for AMD in llvm/llvm-project#89245 .
Functionality of this change LGTM. However, I think we will need the functions to be accompanied by detailed comments. Specifically, the 'SYCLSplitModule' function. This will be required if/when we try to upstream this to llorg. Thanks much Maksim. |
Removes sycl-module-split tool and replaces all usages with llvm-split tool.