[SYCL] Update aspect numbering #14605
Conversation
| __SYCL_ASPECT(ext_oneapi_queue_profiling_tag, 72) | ||
| __SYCL_ASPECT(ext_oneapi_virtual_mem, 73) | ||
| __SYCL_ASPECT(ext_oneapi_cuda_cluster_group, 74) | ||
| __SYCL_ASPECT(ext_oneapi_limited_graph, 63) |
There was a problem hiding this comment.
Why do we need a gap in the numbering here? It seems we're going from 61 directly to 63.
There was a problem hiding this comment.
@steffenlarsen explained to me that we cannot change aspect numbers to avoid breaking existing code.
There was a problem hiding this comment.
Existing code inside the SYCL runtime or user code?
There was a problem hiding this comment.
When task-sequence gets re-submitted we can put it back where it was in that gap.
There was a problem hiding this comment.
The gap is fine, but aspect values should not change. Imagine a kernel that uses double. The compiler will be told that aspect::fp64 is N and stores that with the kernel so the runtime can then check if a kernel is supported on a device. Now, let's say we change fp64 to be N+1. If the user does not recompile their kernel, the runtime will be told that the kernel requires aspect N which is no longer fp64.
There was a problem hiding this comment.
Wouldn't this be allowed in/around an ABI-breaking window? If we allow ABI-break, such an application needs to be re-compiled anyway, right?
I don't want to hold up this specific PR on this issue (in particular if we plan to re-insert the task-sequence into the gap later), so I'm fine with merging. But for the future I think we should have some process/policy defined for this kind of scenario, as it's similar to an ABI-break.
There was a problem hiding this comment.
Wouldn't this be allowed in/around an ABI-breaking window? If we allow ABI-break, such an application needs to be re-compiled anyway, right?
Yes, it should, but it was not approved as an ABI/API break, so I would feel better about reverting it.
I don't want to hold up this specific PR on this issue (in particular if we plan to re-insert the task-sequence into the gap later), so I'm fine with merging. But for the future I think we should have some process/policy defined for this kind of scenario, as it's similar to an ABI-break.
Like changing an enum that crosses library boundaries, I think this would fall under our ABI/API break policy. They are sadly hard to track and sycl::aspect is even worse. Maybe we could have some tests for it. 🤔
This PR reverts the aspect numbering that was modified in PR #14359 to avoid breaking existing code.