[SYCL] Add new aspect ext_oneapi_virtual_functions #15577
Conversation
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| backend BE = getBackend(); | ||
| bool isCompatibleBE = BE == sycl::backend::ext_oneapi_level_zero || | ||
| BE == sycl::backend::opencl; | ||
| return (is_cpu() || is_gpu()) && isCompatibleBE; |
There was a problem hiding this comment.
Is this something we'd want to move to UR eventually?
There was a problem hiding this comment.
What is the criteria of moving things to UR?
The only HW which we don't expect to support function pointers is FPGA. For now we also exclude CUDA & HIP, because we haven't tested those configurations. However, I can imagine that 3rd-party backends may not support necessary SPIR-V extensions we use for virtual functions and therefore the ultimate query should be about SPIR-V extension support here.
There was a problem hiding this comment.
My rule of thumb is: If you have to check for the backend, it should have been done in UR. On a more general note though, my concern is another implementation of L0 or OpenCL could come in and it wouldn't support these, then this would be invalid. In a case like that, it would have to be addressed in the UR adapters anyway.
There was a problem hiding this comment.
My rule of thumb is: If you have to check for the backend, it should have been done in UR.
Good point. Even though I consider exclusion of CUDA & HIP backends temporary, I don't know if there are any other queries that should be performed for those backends (like checking CUDA SM version or something).
Do we have some generic API which we can extend (device info, I assume), i.e. what is the path of extending UR to support such query?
There was a problem hiding this comment.
It would be a query like for aspect::ext_oneapi_virtual_mem a couple of lines above. I don't remember if UR has some tooling to help generate more of these enums. @kbenzie is there?
There was a problem hiding this comment.
It would be a query like for
aspect::ext_oneapi_virtual_mema couple of lines above.
Thanks. Long-term, I think that we should probably move this into UR, but I'm not entirely sure on the name yet. We may add support for generic function pointers later and I believe that aspect for them will be checking for the very same things as we check for virtual functions. Simply because virtual functions are function pointers under the hood.
Therefore, it probably makes sense to have a single generic UR query for both (something like "indirect calls"). However, since we are not yet there with function pointers, I'm hesitant to suggest such generic name just yet.
If there are no objections, I would suggest to have this aspect handled at SYCL level for now and leave a TODO comment to move it into UR later.
There was a problem hiding this comment.
It would be a query like for
aspect::ext_oneapi_virtual_mema couple of lines above. I don't remember if UR has some tooling to help generate more of these enums. @kbenzie is there?
This would entail adding an entry to the ur_device_info_t enum here and runnign the generate target to produce the header and validation layer source changes from that. Then it would be a case of implementing the query in each adapter.
If there are no objections, I would suggest to have this aspect handled at SYCL level for now and leave a TODO comment to move it into UR later.
👍
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KornevNikita
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
KornevNikita
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
|
Pre-commit passed, @steffenlarsen @AlexeySachkov could you please merge? |
Spec: #10540