Skip to content

[SYCL] Separated CompileTimePropertiesPass from sycl-post-link tool #7527

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

Conversation

andylshort
Copy link

@andylshort andylshort commented Nov 24, 2022

Extracted the CompileTimeProperties pass from the sycl-post-link tool up into the llvm directory with the rest of the passes and added it as a step in the code generation pipeline. Moving this into a more generalised location will allow for new tools to be developed that require its logic.

Due to a dependency on DeviceGlobals, that also had to be moved and includes adjusted in sycl-post-link tool. This pass can be invoked via opt as well for testing.

Several sycl-post-link tests had to be transferred to llvm/test/SYCLLowerIR with some being modified as well to account for the change. Some tests had to be split across the two directories so as to maintain coverage. Particular tests that focused on the result of running the pass over some iput bytecode and testing the result have been turned into their own tests under the llvm/test/SYCLLowerIR/CompileTimePropertiesPass test directory.

Lamzed-Short, Andrew added 8 commits November 16, 2022 07:45
DeviceGlobals had to be moved due to the current tight dependency it has
to the properties pass. Maybe this could be addressed later?
This is not its final position, just its current one for testing purposes.
…pass

They're more appropriate as tests for the pass than the tool.
IR checks would fail/be redundant if kept in sycl-post-link test, so
moved them to pass test instead, keeping property checks in old location.
sycl-post-link splits the test into three translation units, if the pass
is run ahead of time, the annotations and properties aren't split properly
so the pass is run after the split per TU to keep the test working.

If the pass can be run before the split, this test can be properly adapted,
but this is the only way I can think to preserve this test and maintain
coverage as is for now.
@andylshort andylshort temporarily deployed to aws January 11, 2023 10:29 — with GitHub Actions Inactive
@andylshort andylshort temporarily deployed to aws January 12, 2023 05:18 — with GitHub Actions Inactive
@andylshort andylshort marked this pull request as ready for review January 12, 2023 11:47
@andylshort andylshort requested review from a team as code owners January 12, 2023 11:47
@andylshort andylshort requested a review from a team January 12, 2023 11:47
Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

FE changes look okay to me.

@andylshort andylshort temporarily deployed to aws January 12, 2023 16:12 — with GitHub Actions Inactive
@andylshort andylshort temporarily deployed to aws January 12, 2023 17:38 — with GitHub Actions Inactive
Copy link
Contributor

@v-klochkov v-klochkov left a comment

Choose a reason for hiding this comment

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

Ok for ESIMD component.

Lamzed-Short, Andrew added 3 commits January 30, 2023 05:41
The pass moving means these are performed earlier and not in sycl-post-link.
This functionality is tested in llvm/test/SYCLLowerIR/CompileTimeProperties/kernel-properties.ll instead.
The test is still required at sycl-post-link splits the source and checks
each individually. Putting the opt invocation before the sycl-post-link
call will remove the CHECK lines, so this way is necessary.
@andylshort andylshort temporarily deployed to aws January 30, 2023 16:44 — with GitHub Actions Inactive
@andylshort andylshort temporarily deployed to aws January 30, 2023 17:47 — with GitHub Actions Inactive
Lamzed-Short, Andrew added 2 commits January 31, 2023 08:57
@andylshort andylshort temporarily deployed to aws January 31, 2023 18:59 — with GitHub Actions Inactive
@andylshort andylshort temporarily deployed to aws January 31, 2023 21:08 — with GitHub Actions Inactive
Copy link
Contributor

@bader bader left a comment

Choose a reason for hiding this comment

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

To be honest, I don't understand the reason to have sycl-post-link passes. I expected that all passes we add for DPC++ should be available for testing via opt tool.

Anyway, this patch looks good to me.

Please, resolve merge conflicts.

@andylshort andylshort temporarily deployed to aws February 2, 2023 14:28 — with GitHub Actions Inactive
@andylshort andylshort temporarily deployed to aws February 2, 2023 15:06 — with GitHub Actions Inactive
@andylshort
Copy link
Author

Current check failure unrelated to PR, failure of some kernel fusion code on OpenCL target.

@AlexeySachkov
Copy link
Contributor

Failed Tests (1):
  SYCL :: KernelFusion/sync_two_queues_requirement.cpp

The failure is unrelated and the test is fixed in intel/llvm-test-suite#1557 to match implementation done in #8148

@AlexeySachkov AlexeySachkov merged commit 355c822 into intel:sycl Feb 2, 2023
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.

8 participants