-
Notifications
You must be signed in to change notification settings - Fork 130
[SYCL][ESIMD] Lsc predicate test #1194
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
kbobrovs
left a comment
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