[SYCL] Add aspect for buffer_location property #5660
Conversation
| namespace sycl { | ||
| enum class aspect { | ||
| ext_intel_buffer_location | ||
| } |
There was a problem hiding this comment.
Rather than adding an aspect, it would be more convenient if the implementation simply ignored the property in cases where it makes no sense for the device. This would be similar to what we did for the init_mode and implement_in_csr properties in sycl_ext_oneapi_device_global.
I think we should look for ways to avoid adding lots of new aspects. Otherwise, it will be very painful for applications because they will need to have tons of apsect-checking tests everywhere.
There was a problem hiding this comment.
Should it be ignored by DPCPP RT or by plugin or by backend?
If the user explicitly specified this property, hence the user knows what he want to do. Would it be correct to change value of the property implicitly? User may also want to use different code in cases when device supports buffer_location feature and when it doesn't.
There was a problem hiding this comment.
Should it be ignored by DPCPP RT or by plugin or by backend?
I don't think it matters from a user perspective.
Would it be correct to change value of the property implicitly?
I don't understand this. Are you asking if it is correct to ignore the property in the case when the device does not support it?
User may also want to use different code in cases when device supports buffer_location feature and when it doesn't.
Rather than having tests for each feature like this, it seems like it would be better to say that features like buffer_location, init_mode, and implement_in_csr are supported on all FPGA devices, and we already have a way for a user to test whether a device is FPGA. In addition, it seems reasonable to ignore these properties when used on a non-FPGA device, since their semantics have no meaning on other devices. This makes it easier for users to pass the same kernel to any device when using these properties.
There was a problem hiding this comment.
I don't understand this. Are you asking if it is correct to ignore the property in the case when the device does not support it?
I meant that if user passed some value to buffer_location property and this property is not supported by device, we will implicitly change this value to default and user will not know about it. Is it correct?
There was a problem hiding this comment.
Yes, I think I agree, but this seems like a strange way of saying it. It would be clearer to say that the bufffer_location property is silently ignored if the device does not support that property.
There was a problem hiding this comment.
I added ignoring of the property in the main PR #5604
There was a problem hiding this comment.
Great. Can you also update the extension specification to state that the buffer_location property is ignored for non-FPGA devices? You can use similar wording as what was added in #5665.
Signed-off-by: mdimakov [email protected]