-
Notifications
You must be signed in to change notification settings - Fork 787
[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
[SYCL] Separated CompileTimePropertiesPass from sycl-post-link tool #7527
Conversation
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.
…t were mainly testing pass functionality.
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.
FE changes look okay to me.
...st/tools/sycl-post-link/device-globals/test_global_variable_many_modules_no_dev_img_scope.ll
Outdated
Show resolved
Hide resolved
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 for ESIMD component.
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.
This is to ensure transforms are being applied.
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.
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.
Current check failure unrelated to PR, failure of some kernel fusion code on OpenCL target. |
The failure is unrelated and the test is fixed in intel/llvm-test-suite#1557 to match implementation done in #8148 |
Extracted the
CompileTimeProperties
pass from thesycl-post-link
tool up into thellvm
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 insycl-post-link
tool. This pass can be invoked via opt as well for testing.Several
sycl-post-link
tests had to be transferred tollvm/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 thellvm/test/SYCLLowerIR/CompileTimePropertiesPass
test directory.