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] Move module splitting functionality from sycl-post-link to SYCLLowerIR #12622

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

maksimsab
Copy link
Contributor

This is a part of migration to New Offloading model and clang-linker-wrapper tool.
The signature of the verifyNoCrossModuleDeviceGlobalUsage is changed so that it returns an Error instead of aborting the executable.

…YCLLowerIR

This is a part of migration to New Offloading model and
clang-linker-wrapper tool.
@maksimsab maksimsab requested review from a team as code owners February 6, 2024 15:31
@maksimsab maksimsab changed the title [SYCL] Move module splitting functionality from sycl-post-link into SYCLLowerIR [SYCL] Move module splitting functionality from sycl-post-link to SYCLLowerIR Feb 6, 2024
@asudarsa
Copy link
Contributor

asudarsa commented Feb 6, 2024

Good refactoring Maksim.

Are there any related tests that need to be moved as well?
Also, can you please state the overall strategy here? Will ModuleSplitting be refactored into an LLVM pass? Also, please add NFC label if needed. May be change in signature makes it non NFC?

Thanks

@maksimsab
Copy link
Contributor Author

@asudarsa
The strategy is to (1) make splitting as a library, (2) use this library in clang-linker-wrapper.
No tests are required here because no new functionality was added. Tests are expected in the second patch that adds use of the library in clang-linker-wrapper.
There is no NFC tag used because I changed the behaviour of error handling (moved abort from Module Splitter to sycl-post-link)
Here in CHECK_AND_EXIT macro:

if (DeviceGlobals) {
auto E = Splitter->verifyNoCrossModuleDeviceGlobalUsage();
CHECK_AND_EXIT(E);
}

@maksimsab
Copy link
Contributor Author

@asudarsa
I pushed the commit that makes the aborting error message the same as it was initial.

@maksimsab
Copy link
Contributor Author

@intel/dpcpp-esimd-reviewers friendly ping.

Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

esimd owned changes lgtm, sorry for late review

@@ -478,13 +484,18 @@ void ModuleSplitterBase::verifyNoCrossModuleDeviceGlobalUsage() {
continue;
}
if (auto *F = dyn_cast<const Function>(U)) {
if (EntryPointModules.count(F))
CheckEntryPointModule(F);
if (EntryPointModules.count(F)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A suggestion.
Going forward it might help if you can split such submissions into multiple commits: (1) Code motion without changes to error handling and (2) Handle changes to error handling. it might make it easy to triage errors later on. Thanks

Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the explanation.

@asudarsa asudarsa merged commit 89eccce into intel:sycl Feb 8, 2024
12 checks passed
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.

4 participants