Host Implementation of Histogram APIs

[danhoeflinger]

Implementation of histogram APIs for host backends.

Implementations are provided for serial, tbb, and openMP backends. We add a generic __thread_enumerable_storage struct to add a generic thread local storage for our host backends. We use the new TLS (Thread local storage) with parallel_for to implement histogram. Testing is also added, and some minor adjustments are made to cmake.

Please see the RFC documentation / discussion here for more details.

[adamfidel]

Looks generally good to me. I had to go through it a few times to understand __construct_by_args, but I think I get how it works now and why it's useful.

Just some minor comments from my side.

[adamfidel]

Is there a reason why _IsVector is a class when the rest are typenames?

[danhoeflinger]

No, it seems the convention in this file is generally for everything to be class (though its not fully consistent). I'll adjust it to the norm.

[adamfidel]

std::uint32_t __count = 0 could probably be moved here to the initialization expression of this loop.

[danhoeflinger]

thanks, done.

[SergeyKopienko]

I think better to use std::size_t for size-related stuff.

[danhoeflinger]

Again, this is associated with the number of threads, is it necessary?

[SergeyKopienko]

From my point of view, if we describe anything about the size, better to use std::size_t type.
The exception - only one case when we save size in the memory and want to reduce the amount of memory.
But of course, I don't insist, it's not one place with another approach.

[SergeyKopienko]

Suggested change

	_DiffType __local_n = __global_histogram_last - __global_histogram_first;
	std::uint32_t __num_temporary_copies = __tls.size();
	_DiffType __range_begin_id = __global_histogram_first - __histogram_first;
	const _DiffType __local_n = __global_histogram_last - __global_histogram_first;
	const std::uint32_t __num_temporary_copies = __tls.size();
	const _DiffType __range_begin_id = __global_histogram_first - __histogram_first;

[mmichel11]

Suggested change

	// Note: Size should not be used concurrantly with parallel loops which may instantiate storage objects, as it may
	// Note: Size should not be used concurrently with parallel loops which may instantiate storage objects, as it may

[mmichel11]

After the first parallel_for, __tls.size() should no longer grow, correct? What about moving __num_temporary_copies outside of this parallel_for and capturing it into the lambda? This way we only perform a single atomic load.

[MikeDvorskiy]

I don't insist... But there is a common good practice to process a shorter branch of if..else operator first. It has better readability. Also, in case of immediate return from the shorter branch we have the "non indented" code of the bigger branch:

if(n ==0)
{
    //shorter branch
   return;
}
//else: bigger branch - no indents.
.....