Skip to content

Update to official oneDPL histogram implementation#2978

Open
danhoeflinger wants to merge 3 commits intooneapi-src:SYCLomaticfrom
danhoeflinger:dev/dhoeflin/histogram_official
Open

Update to official oneDPL histogram implementation#2978
danhoeflinger wants to merge 3 commits intooneapi-src:SYCLomaticfrom
danhoeflinger:dev/dhoeflin/histogram_official

Conversation

@danhoeflinger
Copy link
Contributor

@danhoeflinger danhoeflinger commented Feb 23, 2026

This PR updates the histogram implementation from a temporary feature preview to use the official oneDPL interfaces, and removes the temporary histogram code.

It also removes a size() function from device iterator which causes problems with some internals of oneDPL separating ranges and iterator implementations. This size function seems strange (size of an iterator?), and only appears in one of the device_iterator implementations.

Technically this is removing part of a public interface of device_iterator, but I'm not sure how it makes any sense, and makes usage within oneDPL problematic.

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>
Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>
Copy link
Contributor

@mmichel11 mmichel11 left a comment

Choose a reason for hiding this comment

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

Changes LGTM

d_samples + num_samples, d_histogram,
num_levels - 1, lower_level, upper_level);
oneapi::dpl::histogram(::std::forward<Policy>(policy), d_samples,
d_samples + num_samples, num_levels - 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to run clang-format again here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm, yes. Fixed.

Signed-off-by: Dan Hoeflinger <dan.hoeflinger@intel.com>
Copy link
Contributor

@timmiesmith timmiesmith left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

The iterator.size() method doesn't make sense, but is likely an old workaround. It is enabling the USM-based device_iterator to be treated as a SYCL buffer backed device_iterator. If the iterator is defined over a buffer you can use it.get_buffer().size() to get the size of the input range. The USM-based device_iterator is returning *this for get_buffer() and thus needed a nonsense size method in order for that usage in an algorithm to compile.

@timmiesmith
Copy link
Contributor

CI Windows failures are unrelated to the PR.

@danhoeflinger
Copy link
Contributor Author

danhoeflinger commented Feb 27, 2026

The iterator.size() method doesn't make sense, but is likely an old workaround. It is enabling the USM-based device_iterator to be treated as a SYCL buffer backed device_iterator. If the iterator is defined over a buffer you can use it.get_buffer().size() to get the size of the input range. The USM-based device_iterator is returning *this for get_buffer() and thus needed a nonsense size method in order for that usage in an algorithm to compile.

@timmiesmith
Hmm, this makes some sense but also sets off some alarm bells for me. I'm wondering if we should fully remove the get_buffer which returns *this as well, or if we need to replace it with some proxy which also provides the size() function to the best of its ability. Similar to the removed one here, I don't think we have access to this info in reality.

Do you know where if anywhere this functionality is actually used? Since is_hetero = std::false_type (and it is "passed directly"), I don't expect it to be used within oneDPL at all. I'm not finding any place in SYCLomatic, or the SYCLomatic-tests. There are plenty of get_buffer() calls for oneDPL buffer_wrappers (sycl_iterators), and some for device_vector, but I see none for device_pointer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants