Skip to content

[SYCL] Add aspect for buffer_location property #5660

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

[SYCL] Add aspect for buffer_location property #5660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4421d1a
Add aspect for buffer_location property
maximdimakov Feb 24, 2022
2c59565
Add pi property for buffer location
maximdimakov Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions sycl/doc/extensions/supported/sycl_ext_intel_buffer_location.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,36 @@ It also notifies the SYCL runtime to store the given accessor in that memory. |
|===
--

== Feature test macro

This extension provides a feature-test macro as described in the core SYCL
specification section 6.3.3 "Feature test macros". Therefore, an implementation
supporting this extension must predefine the macro `SYCL_EXT_INTEL_BUFFER_LOCATION` to
one of the values defined in the table below. Applications can test for the
existence of this macro to determine if the implementation supports this
feature, or applications can test the macro’s value to determine which of the
extension’s APIs the implementation supports.

[%header,cols="1,5"]
|===
|Value |Description
|1 |Initial extension version. Base features are supported.
|===

== Extension to `enum class aspect`

[source]
----
namespace sycl {
enum class aspect {
ext_intel_buffer_location
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@maximdimakov maximdimakov Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added ignoring of the property in the main PR #5604

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
----

If device has the `ext_intel_buffer_location` aspect, then its Device-Side
Runtime is capable of allocating buffer in different locations.

== Issues

== Revision History
Expand Down
1 change: 1 addition & 0 deletions sycl/include/CL/sycl/aspects.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ enum class aspect {
ext_oneapi_native_assert = 31,
host_debuggable = 32,
ext_intel_gpu_hw_threads_per_eu = 33,
ext_intel_buffer_location = 34,
};

} // namespace sycl
Expand Down
3 changes: 2 additions & 1 deletion sycl/include/CL/sycl/detail/pi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,8 @@ typedef enum {
PI_EXT_ONEAPI_DEVICE_INFO_MAX_GLOBAL_WORK_GROUPS = 0x20000,
PI_EXT_ONEAPI_DEVICE_INFO_MAX_WORK_GROUPS_1D = 0x20001,
PI_EXT_ONEAPI_DEVICE_INFO_MAX_WORK_GROUPS_2D = 0x20002,
PI_EXT_ONEAPI_DEVICE_INFO_MAX_WORK_GROUPS_3D = 0x20003
PI_EXT_ONEAPI_DEVICE_INFO_MAX_WORK_GROUPS_3D = 0x20003,
PI_MEM_PROPERTIES_ALLOC_BUFFER_LOCATION = CL_MEM_ALLOC_BUFFER_LOCATION_INTEL
} _pi_device_info;

typedef enum {
Expand Down
1 change: 1 addition & 0 deletions sycl/include/CL/sycl/feature_test.hpp.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ namespace sycl {
#if SYCL_BUILD_PI_HIP
#define SYCL_EXT_ONEAPI_BACKEND_HIP 1
#endif
#define SYCL_EXT_INTEL_BUFFER_LOCATION 1

} // namespace sycl
} // __SYCL_INLINE_NAMESPACE(cl)
5 changes: 5 additions & 0 deletions sycl/source/detail/device_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,11 @@ bool device_impl::has(aspect Aspect) const {
MDevice, PI_DEVICE_INFO_GPU_HW_THREADS_PER_EU,
sizeof(pi_device_type), &device_type,
&return_size) == PI_SUCCESS;
case aspect::ext_intel_buffer_location:
return getPlugin().call_nocheck<detail::PiApiKind::piDeviceGetInfo>(
MDevice, PI_MEM_PROPERTIES_ALLOC_BUFFER_LOCATION,
sizeof(pi_device_type), &device_type,
&return_size) == PI_SUCCESS;
case aspect::ext_intel_device_info_uuid: {
auto Result = getPlugin().call_nocheck<detail::PiApiKind::piDeviceGetInfo>(
MDevice, PI_DEVICE_INFO_UUID, 0, nullptr, &return_size);
Expand Down