llvm
llvm copied to clipboard
intel
[sycl-post-link] Allow deletion of 'entry point' functions after full inlining.
@kbobrovs We have some functions that get marked with 'always_inline' attribute inside lower ESIMD pass even though they have been marked as entry points. This patch adds 'noinline' attribute to all entry points to avoid this. Can you please comment? Thanks
@kbobrovs We have some functions that get marked with 'always_inline' attribute inside lower ESIMD pass even though they have been marked as entry points.
This sounds like an issue, which we should fix inside lower ESIMD pass or somewhere else. Adding no-inline attribute to all kernels will impact performance of non-ESIMD kernels as well.
Adding no-inline attribute to all kernels will impact performance of non-ESIMD kernels as well.
@bader, I think this attribute only matters for entry points which are not kernels, but SYCL_EXTERNAL functions, as kernels are not callable/inlineable from the device code. But with this correction your concern seems valid. The patch could be modified to only add such attribute to ESIMD functions. But, as commented above, I think better fix would be not removing entry points after inlining.
@asudarsa, as we talked, this fix seems conceptually sound, but it might have performance implications. Your suggestion to try to inline SYCL_EXTERNAL functions w/o deleting the inlined function seems the cleanest solution if it is possible.
According to my understanding it's illegal to delete functions annotated with SYCL_EXTERNAL. These functions have external linkage and can be referenced in other translation units and deleting them can cause linkage issues.
Adding no-inline attribute to all kernels will impact performance of non-ESIMD kernels as well.
@bader, I think this attribute only matters for entry points which are not kernels,
Let's make sure we use the same terminology. "entry point" in SYCL/SPIR-V is by definition can be kernels only. I think what you mean here are functions visible by other translation units. Am I right?
but SYCL_EXTERNAL functions, as kernels are not callable/inlineable from the device code. But with this correction your concern seems valid. The patch could be modified to only add such attribute to ESIMD functions. But, as commented above, I think better fix would be not removing entry points after inlining.
That's right. My main concern is the impact on non-ESIMD kernels.
I also slightly concerned about the approach used for ESIMD kernels. I don't know the reasoning behind adding always_inline/noinline attributes implicitly, but it sounds like an easy way to shoot ourselves in the foot. Does regular inliner pass a poor job? Is it possible to add these attributes explicitly in the headers to avoid unexpected compiler behavior?
Hi @bader Thanks so much for your comments. Based on all the comments, I am convinced that this PR is a workaround for a much bigger problem. The issue seems to be an error in user code. In the test-case I was looking at, there is a member function definition inside the class definition. And it has been marked with 'SYCL_EXPLICIT' attribute. That seems like a mismatch. This PR will try to let such mismatches pass through. Marking this as a Draft till we hear back from user. Thanks again