Fixed issues encountered through oneMKL portBLAS backend #504
Conversation
OuadiElfarouki
requested review from
muhammad-tanvir-1211,
s-Nick,
pgorlani and
aacostadiaz
|
Thanks for your PR, @s-Nick. Could you please update the PR comment including your modification to |
| // Skip data initialization if not accessing in read mode only | ||
| return typename BufferIterator<element_t>::template accessor_t<acc_md_t>( | ||
| buffer_, cgh, cl::sycl::range<1>(size), | ||
| cl::sycl::id<1>(BufferIterator<element_t>::get_offset()), | ||
| cl::sycl::property::no_init{}); |
There was a problem hiding this comment.
Does this have a performance impact for devices where this code was originally working?
There was a problem hiding this comment.
@hjabird This was actually added to get rid of some warnings regarding buffers initialization when using AdaptiveCpp, but introducing it caused some tests to fail through the portBLAS backend API in oneMKL, that's why we're reverting. No performance aspects have been tested/verified but this change was tested with AdaptiveCpp (warnings are back but no errors/tests failures).
| # * @filename CMakeLists.txt | ||
| # * | ||
| # **************************************************************************/ | ||
| cmake_minimum_required(VERSION 3.4.3) |
There was a problem hiding this comment.
I'm curious about the choice of 3.4.3 - has this been tested?
There was a problem hiding this comment.
Yes I just used the same version required by the main CMakeLists.txt, as I got some warnings/errors if I still remember when building the samples alone (header only with portBLAS)
…header only lib behaviors
AMD atomic operation implementation requires some specific hardware to work properly with current reduction kernel. This patch adds a check for AMD only to provides the correct result even if the specific hardware is not available. Signed-off-by: nscipione <nicolo.scipione@codeplay.com>
Signed-off-by: nscipione <nicolo.scipione@codeplay.com>
buffer only Signed-off-by: nscipione <nicolo.scipione@codeplay.com>
Add documentation for `usmManagedMem` template parameter. Signed-off-by: nscipione <nicolo.scipione@codeplay.com>
Co-authored-by: HJA Bird <hja.bird@gmail.com> Co-authored-by: pgorlani <92453485+pgorlani@users.noreply.github.com>
comments Addressing PR comments on memory allocation type function checker and updating comment. Fixing documentation. Signed-off-by: nscipione <nicolo.scipione@codeplay.com>
s-Nick
force-pushed
the
acpp_regression_fixes
branch
from
f1b4ee3 to
aca6ee0
Compare
This patch includes following changes :
BufferIterator<T>::get_range_accessor()that caused some accessors to be not initialized (sycl::property::no_init{}) and thus yield erroneous test results.WGAtomicReductionsignature to comply with different usm memory allocation and atomic initialization. AddingusmManagedMemto flag if memory is allocated using managed memory which requiresgeneric_spacein atomic initialization, especially on AMD hardware. Flag added not to cause performance degradation to the operators that use this kernel.