RFC for histogram CPU implementation

[danhoeflinger]

Adds an RFC for histogram CPU implementation.

[akukanov]

A couple of papers that might be of interest:

• https://www.researchgate.net/publication/221131224_SIMD_Vectorization_of_Histogram_Functions
• https://www.researchgate.net/publication/266660552_Versatile_and_scalable_parallel_histogram_construction

[danhoeflinger]

Thanks, these are interesting, and mainly follow similar lines to the approaches outlined here. I will look more into the pcwar SIMD instruction to understand if that is viable to use here.

[danhoeflinger]

While its interesting to explore, I believe we basically depend on OpenMP simd to provide our SIMD operations. We won't be able to serve more complex intrinsic operations if we want to stick to that.

Further, I'm not actually finding anything like the pcwar instruction(s) they refer to here in https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html. I do find some for conflict detection across lane which could be useful, but again those won't be available through the interface of OpenMP.

I think our options are basically to decline SIMD or to have duplicates of the output for each SIMD lane. I think even that may be more "hands on" with SIMD details than we have done thus far from oneDPL.

[akukanov]

Overall, this all sounds good enough for the "proposed" stage, where it's expected that some details are unknown and need to be determined. I am happy to approve it but will wait for a few days in case @danhoeflinger wants to update the document with some follow-up thoughts on the discussion.

[danhoeflinger]

Overall, this all sounds good enough for the "proposed" stage, where it's expected that some details are unknown and need to be determined. I am happy to approve it but will wait for a few days in case @danhoeflinger wants to update the document with some follow-up thoughts on the discussion.

I've made a few more minor adjustments, but I think I'm happy with the state of the RFC now. The question I have is what is the next best step once we merge. I will be working on the rough implementation (already in progress), but should I also open a new draft PR moving this from proposed to supported as a location for further discussion? Or should I merely open PRs in-place if there are significant changes motivated by the experience in implementation?

[akukanov]

I approve it for the implementation as a supported functionality, with the following understanding of the proposed design:

• For multithreaded execution with par and par_unseq, the implementation uses the existing parallel_for backend pattern.
• Unsequenced policies do not enable vectorization when thread-local histograms are built (the 1st stage of the algorithm) but do apply for combining those histograms into the final result (the 2nd stage).

The following questions I believe will need to be answered:

• Which extensions are needed in the parallel backend API to facilitate the use of enumerable storage for per-thread temporary buffers?
• Will the serial backend also allocate a temporary buffer, or will it directly accumulate into the output sequence and run just the 1st stage? If the latter - how this difference between backends will be handled on the pattern level?
• Should the 2nd stage always run according to the given execution policy, or should it under certain conditions better run serially by the calling thread?

Specification changes are not required.

I would recommend @MikeDvorskiy as the second approver.

[MikeDvorskiy]

let me share my thoughts:
In my understanding RFC is not a book... So, I would preferer to have a short, concise and precise description of what is offered, without frills, like a mathematical theorem. For example:

"The oneDPL library added histogram APIs, currently implemented only for device policies with the DPC++ backend. These APIs are defined in the oneAPI Specification 1.4. Please see the
oneAPI Specification for the details. The host-side backends (serial, TBB, OpenMP) are not yet supported. This RFC proposes extending histogram support to these backends."

[danhoeflinger]

Yes, I've accepted your language here. Thanks.

[MikeDvorskiy]

Due to it is not a story telling, I would suggest omitting introductory expressions like "It may make more sense" or "It's natural for a user to expect"... Only short and exact information.

For example,
"There are many cases to use a host-side serial or a host-side implementation of histogram. Another motivation for adding the support is simply to be spec compliant with the oneAPI specification."

[danhoeflinger]

taken suggestion. Thanks

[MikeDvorskiy]

Taking into account my shared thought above, I would propose to re-prahse it keeping the main point shorter:

"A histogram algorithm is a memory-bound algorithm. So, the implementation should care of reducing memory accesses and minimizing temporary memory traffic."

[danhoeflinger]

Taken mostly. Thanks

[danhoeflinger]

I plan to do a pass cleaning up and also addressing some of what Alexey mentioned above today / tomorrow morning. I will also try to cut down verbosity.

I also have a draft PR up for the implementation which is a work in progress. #1974
I'll add this link to the RFC with my next changes.

[MikeDvorskiy]

I guess we can this topic omit at all. It tells nothing about 'histogram', just general wording, which can be applied for any new feature in oneDPL...

[danhoeflinger]

I've removed some of the general language and added something which is important for histogram in an attempt to answer feedback from @akukanov to clarify where the implementation of the algorithm will live.

[MikeDvorskiy]

"unseq Backend"
Basically, we don't have such back end officially. Yes, sometimes we used such term in the internal communication as for "name" for a set of functions with "pragma simd" implementation. But we did not specify and publish API for that. So, I suggest renaming this topic to "SIMD/openMP SIMD Implementation" f.e.

[akukanov]

I think this part is about developing (or not) an implementation for unsequenced policies.
I do not mind calling it `unseq backend" in the design docs, but Mikhail is correct that it's rather informal (while parallel backend is somewhat more formal).

[danhoeflinger]

I like the proposed name for the section better anyway for what is discussed. Thanks.

[MikeDvorskiy]

https://github.com/oneapi-src/oneDPL/pull/1930/files#diff-fb5f6394ad0d350d719b9f31b139fa60c347ec64795c78e56875c4f002aeb0e7R25
We already have the key requirements topic where we enumerate all backends that we propose to support.
It is good enough I think, and we also can omit this topic "Serial Backend".

Explanation what is "Serial Backend" means as the others backends mean, is a kind of "oneDPL general description" and not related to RFC for histogram feature, IMHO.

[danhoeflinger]

With some recent changes, there is some specifics about the serial implementation I wanted to add here so I've kept the section.

[MikeDvorskiy]

If we intent to give some information about OneDPL parallel backend patterns on which histogram can based on, I would notify, there is not "count_if" pattern, there is "reduce"("transform_reduce") pattern.
When a man says "reduce", it becomes more or less obvious that histogram calculation based on reduce is not effective at all.

[danhoeflinger]

I clarified the language a little here to make it more clear that copy_if uses reduce internally. I still think it deserves some text describing it as it may not be immediately obvious to everyone that reduce is not well matched.

[MikeDvorskiy]

BTW, I have a curiosity question. Which approach does NVidia use?

[danhoeflinger]

NVidia has a similar API within CUB but not within Thrust, and therefore does not have a CPU implementation that I am aware of, only one specifically for a GPU device.

[MikeDvorskiy]

What does Embarrassingly Parallel term mean?

[MikeDvorskiy]

1. Update: got it.. https://en.wikipedia.org/wiki/Embarrassingly_parallel

2. Of course, if you are solving a concrete task and you are allowed to use the all machine recourses, and there are no any other workloads on the node, the best way for histogram calculation - to make static dividing of amount of work, each thread is calculating a local histogram, and after the local histograms are reducing into one.

3. But, talking about parallelism in a kind of general library we have to keep in mind that a final user's application can work in "different circumstances", depends on their application type, task, real-time data, other workloads on the same host and other many things..
   When we were developing TBB backend we kept in mind that things and preferred to use TBB auto partitioner (instead of static f.e).
   Also composability reasons make sense here.

4. BTW, have you considered a "common parallel reduce" (in general) pattern (and tbb::parallel_reduce pattern, in particular) for histogram calculation? It seems the parallel histogram calculation matches on the common reduce (with a certain "big" grainsize): each Body calculates a local histogram (bins), Combiner summaries the all local bins into final ones.
   Additionally, if number of bins is "big" we can apply the second level of parallelism within Combiner code - SIMD or even "parallel_for" and SIMD, if number of bins is "too big".

[danhoeflinger]

2. Yes, although I think there is no reason here to do static division of work, but rather rely upon our existing parallel_for implementation to provide a good composable implementation.

3. Agree, which is why the intent is to use the existing parallel_for structure (including partitioners) to implement the parallelism. If we were to do it from scratch, we would do it in a similarly composable way, but better to rely upon existing infrastructure

4. Yes, I thought about this. For TBB and even more for openMP the built in reduction functionality is geared toward very simple lightweight types as the reduction variable where we may have an arbitrarily large array. Especially since we want a unified implementation, it does not seem like these backend are really set up to handle these large reduction variables. It seems we should take more control to ensure no unnecessary copies are made, and that the final combination is done performantly, based on knowledge we have of the task. The implementation remains quite simple and unified.

[MikeDvorskiy]

1. Why a TLS is used, not the global memory with "thread id" as a key? F.e. bins[thread_id]?
2. Does TBB guarantee that the same threads finalize the work? "The same threads" means the threads which have started the work?

[danhoeflinger]

1. There are a number of reasons:
   a) My understanding is that in TBB while it may be technically possible to get the the local thread id within an arena, it is an undocumented API and generally discouraged and against the TBB mindset. Using TLS seems to be the preferred method specifically with TBB.
   b) While what you suggest perhaps fits better within OpenMP, we want to create a single implementation and not require a __parallel_histogram within every current and future backend, but rather depend upon existing functionality within the backend as much as we can (in this case __parallel_for).
   c) With smaller values of n, num_bins and larger number of threads, not all threads should be used because of the overhead associated with allocation and initialization of each temporary bin copy. We can let the partitioner decide how many blocks to employ, but we want to avoid unnecessary allocation and initialization overheads wherever possible.

I will mention a downside for completeness, but it is outweighed here in my opinion:
It requires implementation of a thread local storage class for each backend. This is only non-trivial for OpenMP. It has been written generically though to serve future patterns though so it is nice to have.

[danhoeflinger]

2. I'm not exactly sure what you mean here by "finalize the work". If you mean the second parallel for, then no, we are explicitly parallelizing over a different dimension (num_bins), and accumulating across the temporary histograms which were used from different threads. TBB does guarantee that each thread will always use its own TLS for each grain of work though, when retrieved through local().