-
Notifications
You must be signed in to change notification settings - Fork 130
Conversation
/verify with intel/llvm#6688 |
SYCL/ESIMD/lsc/lsc_predicate.cpp
Outdated
@@ -0,0 +1,200 @@ | |||
//==------------ lsc_neg.cpp - DPC++ ESIMD on-device test ------------------==// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong filename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
// | ||
//===----------------------------------------------------------------------===// | ||
// REQUIRES: gpu-intel-pvc || esimd_emulator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add description what the test does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
SYCL/ESIMD/lsc/lsc_predicate.cpp
Outdated
// | ||
//===----------------------------------------------------------------------===// | ||
// REQUIRES: gpu-intel-pvc || esimd_emulator | ||
// UNSUPPORTED: cuda || hip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
//===----------------------------------------------------------------------===// | ||
// REQUIRES: gpu-intel-pvc || esimd_emulator | ||
// UNSUPPORTED: cuda || hip | ||
// RUN: %clangxx -fsycl %s -o %t.out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please create a *_stateless variant of the test - see e.g. llvm-test-suite/SYCL/ESIMD/lsc/lsc_surf_stateless.cpp
(use -fsycl-esimd-force-stateless-mem to compile)
Otherwise this test does not catch a bug in the predicate implementation PR (intel/llvm#6688)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
SYCL/ESIMD/lsc/lsc_predicate.cpp
Outdated
lsc_block_store<int, SIMDSize>(access_0, offset, data_0 * 2, | ||
pred_enable); | ||
|
||
lsc_prefetch<int, SIMDSize, lsc_data_size::default_size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please try to use only minimal set of APIs. lsc_prefetch
is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
offset); | ||
auto data_2 = | ||
lsc_block_load<int, SIMDSize>(access_2, offset, pred_enable); | ||
lsc_block_store<int, SIMDSize>(access_2, offset, data_2 * 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have 4 loads and 4 stores - do they test some different aspects of the API? If not, please minimize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to verify all possible combinations of predicate flags. Not sure it is strictly necessary but won't hurt.
SYCL/ESIMD/lsc/lsc_predicate.cpp
Outdated
|
||
int testUSM(queue q) { | ||
auto size = size_t{128}; | ||
auto constexpr SIMDSize = unsigned{4}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not a practical SIMD size. 8,16,32 would be more relevant. Or, even better, you can templatize by SIMD size and check all of them - like we usually do in other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address comments.
* Add test for lsc block load/store "whole operation" predicates.
[SYCL][ESIMD] Lsc block load/store predicate test (intel#1194)
…ite#1194) * Add test for lsc block load/store "whole operation" predicates.
Complementary PR for: intel/llvm#6688