SYCL RangePolicy: manually specify workgroup size through chunk size

[masterleinad]

In some cases, selecting a custom workgroup size gives significantly better performance than relying on the compiler to choose.

[pvelesko]

With the changes I left in the review, the code runs correctly and now outperforms Kokkos::OpenMPTarget for my example

[masterleinad]

CUDA-10.1-Clang-Tidy timing out is clearly unrelated.

[nmm0]

lgtm pending approval by @pvelesko

[masterleinad]

Retest this please.

[crtrott]

We need to have a discussion about this. Note that what you use launch bounds for here, is NOT the intended use, and is NOT the effect you get on either HIP or CUDA. I.e. it does not actually set ranges. If people now start using it for that purpose because of SYCL we potentially create issues. I'd rather have you use chunk_size or something like that.

[masterleinad]

We need to have a discussion about this. Note that what you use launch bounds for here, is NOT the intended use, and is NOT the effect you get on either HIP or CUDA. I.e. it does not actually set ranges. If people now start using it for that purpose because of SYCL we potentially create issues. I'd rather have you use chunk_size or something like that.

Sure, it's slightly abusing the concept. My thought was that it fits close enough to what I'm trying to do here since it's an optimization hint (that is actually taking into account). Also, the values for LaunchBounds are normally backend-specific and I don't see any other use for LaunchBounds in SYCL (upcoming). Anyway, I'm happy to discuss alternatives. Note that this is for RangePolicy where we don't have chunk_size.

[dalg24]

Actually we do (I had to look it up)

kokkos/core/src/Kokkos_ExecPolicy.hpp

Lines 201 to 207 in e765d32

	/** \brief set chunk_size to a discrete value*/
	inline RangePolicy set_chunk_size(int chunk_size_) const {
	RangePolicy p = *this;
	p.m_granularity = chunk_size_;
	p.m_granularity_mask = p.m_granularity - 1;
	return p;
	}

and, as a side comment, I don't quite understand why we don't just update *this and return a reference to it.

[masterleinad]

OK, fair enough.

[crtrott]

what happens if maxTperB is not set (i.e. -1 I think??

[masterleinad]

Fixed.

[crtrott]

@masterleinad we need some clear documentation for this, since this in particular is different from how OpenMP works, where chunksize is the number of consecutive iterations given to a single thread. However, practically always the only performant choice is 1, so that usage of chunk size is kinda useless anyway and reinterpreting it as something like the block size/work group size is probably not a bad idea. We should consider doing this for the other backends I guess.

[dalg24]

Title line is misleading because it only applies to parallel_for, not parallel_reduce.
Please comment whether this can/should be implemented for other parallel constructs with a RangePolicy.

[masterleinad]

Please comment whether this can/should be implemented for other parallel constructs with a RangePolicy.

paralle_for was just the most relevant case, but I intended to add versions for other parallel constructs after merging this when I opened the pull request. Since we want to discuss what we want to do with different backends anyway, I would wait with that, though.