Skip to content

Commit 3f17ada

Browse files
againullagainull
authored
[SYCL][NFC] Fix bugs in unit tests (#7070)
* Currently PIImage object holds raw pointers to the data in std containers in the MBinaryDesc structure. If PIImage object gets moved or copied then these pointers get invalidated causing invalid memory access and flaky failures. Patch removes MBinaryDesc structure and changes the code to get those pointers on the fly. * Remove cend() iterator dereference in unit tests. * Use different kernel identifiers for different tests. All device images are stored in the same program manager object and tests doesn't take that into account. So, using the same kernel identifier in different tests leads to multiple device images (related to different tests) in the program manager associated with the same kernel identifier. It leads to the situation when test's behaviour is affected by artifacts from another test. * Fixed redefined compile/link/build plugin functions. Currently they may produce a result with redundant space symbol.
1 parent ffc85b8 commit 3f17ada

File tree

3 files changed

+72
-78
lines changed

3 files changed

+72
-78
lines changed

‎sycl/unittests/helpers/PiImage.hpp‎

Lines changed: 25 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -202,33 +202,11 @@ class PiImage {
202202
const std::string &CompileOptions, const std::string &LinkOptions,
203203
std::vector<char> Manifest, std::vector<unsigned char> Binary,
204204
PiArray<PiOffloadEntry> OffloadEntries, PiPropertySet PropertySet)
205-
: MDeviceTargetSpec(DeviceTargetSpec), MCompileOptions(CompileOptions),
205+
: MVersion(Version), MKind(Kind), MFormat(Format),
206+
MDeviceTargetSpec(DeviceTargetSpec), MCompileOptions(CompileOptions),
206207
MLinkOptions(LinkOptions), MManifest(std::move(Manifest)),
207208
MBinary(std::move(Binary)), MOffloadEntries(std::move(OffloadEntries)),
208-
MPropertySet(std::move(PropertySet)) {
209-
auto [ManifestStart,
210-
ManifestEnd] = [this]() -> std::pair<const char *, const char *> {
211-
if (!MManifest.empty())
212-
return {&*MManifest.cbegin(), &*MManifest.cend()};
213-
return {nullptr, nullptr};
214-
}();
215-
MBinaryDesc = pi_device_binary_struct{
216-
Version,
217-
Kind,
218-
Format,
219-
MDeviceTargetSpec.c_str(),
220-
MCompileOptions.c_str(),
221-
MLinkOptions.c_str(),
222-
ManifestStart,
223-
ManifestEnd,
224-
&*MBinary.begin(),
225-
(&*MBinary.begin()) + MBinary.size(),
226-
MOffloadEntries.begin(),
227-
MOffloadEntries.end(),
228-
MPropertySet.begin(),
229-
MPropertySet.end(),
230-
};
231-
}
209+
MPropertySet(std::move(PropertySet)) {}
232210

233211
/// Constructs a SYCL device image of the latest version.
234212
PiImage(uint8_t Format, const std::string &DeviceTargetSpec,
@@ -240,17 +218,36 @@ class PiImage {
240218
std::move(Binary), std::move(OffloadEntries),
241219
std::move(PropertySet)) {}
242220

243-
pi_device_binary_struct convertToNativeType() const { return MBinaryDesc; }
221+
pi_device_binary_struct convertToNativeType() {
222+
return pi_device_binary_struct{
223+
MVersion,
224+
MKind,
225+
MFormat,
226+
MDeviceTargetSpec.c_str(),
227+
MCompileOptions.c_str(),
228+
MLinkOptions.c_str(),
229+
MManifest.empty() ? nullptr : &*MManifest.cbegin(),
230+
MManifest.empty() ? nullptr : &*MManifest.crbegin() + 1,
231+
&*MBinary.begin(),
232+
(&*MBinary.begin()) + MBinary.size(),
233+
MOffloadEntries.begin(),
234+
MOffloadEntries.end(),
235+
MPropertySet.begin(),
236+
MPropertySet.end(),
237+
};
238+
}
244239

245240
private:
241+
uint16_t MVersion;
242+
uint8_t MKind;
243+
uint8_t MFormat;
246244
std::string MDeviceTargetSpec;
247245
std::string MCompileOptions;
248246
std::string MLinkOptions;
249247
std::vector<char> MManifest;
250248
std::vector<unsigned char> MBinary;
251249
PiArray<PiOffloadEntry> MOffloadEntries;
252250
PiPropertySet MPropertySet;
253-
pi_device_binary_struct MBinaryDesc;
254251
};
255252

256253
/// Convenience wrapper around pi_device_binaries_struct, that manages mock
@@ -259,7 +256,7 @@ template <size_t __NumberOfImages> class PiImageArray {
259256
public:
260257
static constexpr size_t NumberOfImages = __NumberOfImages;
261258

262-
PiImageArray(const PiImage *Imgs) {
259+
PiImageArray(PiImage *Imgs) {
263260
for (size_t Idx = 0; Idx < NumberOfImages; ++Idx)
264261
MNativeImages[Idx] = Imgs[Idx].convertToNativeType();
265262

‎sycl/unittests/kernel-and-program/KernelBuildOptions.cpp‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,19 +17,19 @@
1717

1818
#include <gtest/gtest.h>
1919

20-
class TestKernel;
20+
class BuildOptsTestKernel;
2121

2222
static std::string BuildOpts;
2323
namespace sycl {
2424
__SYCL_INLINE_VER_NAMESPACE(_V1) {
2525
namespace detail {
26-
template <> struct KernelInfo<TestKernel> {
26+
template <> struct KernelInfo<BuildOptsTestKernel> {
2727
static constexpr unsigned getNumParams() { return 0; }
2828
static const kernel_param_desc_t &getParamDesc(int) {
2929
static kernel_param_desc_t Dummy;
3030
return Dummy;
3131
}
32-
static constexpr const char *getName() { return "TestKernel"; }
32+
static constexpr const char *getName() { return "BuildOptsTestKernel"; }
3333
static constexpr bool isESIMD() { return true; }
3434
static constexpr bool callsThisItem() { return false; }
3535
static constexpr bool callsAnyThisFreeFunction() { return false; }
@@ -91,7 +91,7 @@ static sycl::unittest::PiImage generateDefaultImage() {
9191
addESIMDFlag(PropSet);
9292
std::vector<unsigned char> Bin{0, 1, 2, 3, 4, 5}; // Random data
9393

94-
PiArray<PiOffloadEntry> Entries = makeEmptyKernels({"TestKernel"});
94+
PiArray<PiOffloadEntry> Entries = makeEmptyKernels({"BuildOptsTestKernel"});
9595

9696
PiImage Img{PI_DEVICE_BINARY_TYPE_SPIRV, // Format
9797
__SYCL_PI_DEVICE_BINARY_TARGET_SPIRV64, // DeviceTargetSpec
@@ -118,8 +118,10 @@ TEST(KernelBuildOptions, KernelBundleBasic) {
118118

119119
const sycl::context Ctx = Queue.get_context();
120120

121+
auto KernelID = sycl::get_kernel_id<BuildOptsTestKernel>();
121122
sycl::kernel_bundle KernelBundle =
122-
sycl::get_kernel_bundle<sycl::bundle_state::input>(Ctx, {Dev});
123+
sycl::get_kernel_bundle<sycl::bundle_state::input>(Ctx, {Dev},
124+
{KernelID});
123125
auto ExecBundle = sycl::build(KernelBundle);
124126
EXPECT_EQ(BuildOpts,
125127
"-compile-img -vc-codegen -disable-finalizer-msg -link-img");

‎sycl/unittests/program_manager/passing_link_and_compile_options.cpp‎

Lines changed: 40 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,10 @@ class EAMTestKernel2;
2525
const char EAMTestKernelName2[] = "LinkCompileTestKernel2";
2626
constexpr unsigned EAMTestKernelNumArgs2 = 4;
2727

28+
class EAMTestKernel3;
29+
const char EAMTestKernelName3[] = "LinkCompileTestKernel3";
30+
constexpr unsigned EAMTestKernelNumArgs3 = 4;
31+
2832
namespace sycl {
2933
__SYCL_INLINE_VER_NAMESPACE(_V1) {
3034
namespace detail {
@@ -54,13 +58,26 @@ template <> struct KernelInfo<EAMTestKernel2> {
5458
static constexpr int64_t getKernelSize() { return 1; }
5559
};
5660

61+
template <> struct KernelInfo<EAMTestKernel3> {
62+
static constexpr unsigned getNumParams() { return EAMTestKernelNumArgs3; }
63+
static const kernel_param_desc_t &getParamDesc(int) {
64+
static kernel_param_desc_t Dummy;
65+
return Dummy;
66+
}
67+
static constexpr const char *getName() { return EAMTestKernelName3; }
68+
static constexpr bool isESIMD() { return false; }
69+
static constexpr bool callsThisItem() { return false; }
70+
static constexpr bool callsAnyThisFreeFunction() { return false; }
71+
static constexpr int64_t getKernelSize() { return 1; }
72+
};
73+
5774
} // namespace detail
5875
} // __SYCL_INLINE_VER_NAMESPACE(_V1)
5976
} // namespace sycl
6077

6178
template <typename T>
6279
static sycl::unittest::PiImage
63-
generateEAMTestKernel1Image(std::string _cmplOptions, std::string _lnkOptions) {
80+
generateEAMTestKernelImage(std::string _cmplOptions, std::string _lnkOptions) {
6481
using namespace sycl::unittest;
6582

6683
std::vector<unsigned char> KernelEAM1{0b00000101};
@@ -75,36 +92,8 @@ generateEAMTestKernel1Image(std::string _cmplOptions, std::string _lnkOptions) {
7592

7693
std::vector<unsigned char> Bin{0, 1, 2, 3, 4, 5}; // Random data
7794

78-
PiArray<PiOffloadEntry> Entries = makeEmptyKernels({EAMTestKernelName1});
79-
80-
PiImage Img{PI_DEVICE_BINARY_TYPE_SPIRV, // Format
81-
__SYCL_PI_DEVICE_BINARY_TARGET_SPIRV64, // DeviceTargetSpec
82-
_cmplOptions, // Compile options
83-
_lnkOptions, // Link options
84-
std::move(Bin),
85-
std::move(Entries),
86-
std::move(PropSet)};
87-
return Img;
88-
}
89-
90-
template <typename T>
91-
static sycl::unittest::PiImage
92-
generateEAMTestKernel2Image(std::string _cmplOptions, std::string _lnkOptions) {
93-
using namespace sycl::unittest;
94-
95-
std::vector<unsigned char> KernelEAM2{0b00000101};
96-
PiProperty EAMKernelPOI =
97-
makeKernelParamOptInfo(sycl::detail::KernelInfo<T>::getName(),
98-
EAMTestKernelNumArgs2, KernelEAM2);
99-
PiArray<PiProperty> ImgKPOI{std::move(EAMKernelPOI)};
100-
101-
PiPropertySet PropSet;
102-
PropSet.insert(__SYCL_PI_PROPERTY_SET_KERNEL_PARAM_OPT_INFO,
103-
std::move(ImgKPOI));
104-
105-
std::vector<unsigned char> Bin{0, 1, 2, 3, 4, 5}; // Random data
106-
107-
PiArray<PiOffloadEntry> Entries = makeEmptyKernels({EAMTestKernelName2});
95+
PiArray<PiOffloadEntry> Entries =
96+
makeEmptyKernels({sycl::detail::KernelInfo<T>::getName()});
10897

10998
PiImage Img{PI_DEVICE_BINARY_TYPE_SPIRV, // Format
11099
__SYCL_PI_DEVICE_BINARY_TARGET_SPIRV64, // DeviceTargetSpec
@@ -122,9 +111,12 @@ inline pi_result redefinedProgramLink(pi_context, pi_uint32, const pi_device *,
122111
void (*)(pi_program, void *), void *,
123112
pi_program *) {
124113
assert(_linkOpts != nullptr);
125-
if (!current_link_options.empty())
126-
current_link_options += " ";
127-
current_link_options += std::string(_linkOpts);
114+
auto add_link_opts = std::string(_linkOpts);
115+
if (!add_link_opts.empty()) {
116+
if (!current_link_options.empty())
117+
current_link_options += " ";
118+
current_link_options += std::string(_linkOpts);
119+
}
128120
return PI_SUCCESS;
129121
}
130122

@@ -134,9 +126,12 @@ inline pi_result redefinedProgramCompile(pi_program, pi_uint32,
134126
const pi_program *, const char **,
135127
void (*)(pi_program, void *), void *) {
136128
assert(_compileOpts != nullptr);
137-
if (!current_compile_options.empty())
138-
current_compile_options += " ";
139-
current_compile_options += std::string(_compileOpts);
129+
auto add_compile_opts = std::string(_compileOpts);
130+
if (!add_compile_opts.empty()) {
131+
if (!current_compile_options.empty())
132+
current_compile_options += " ";
133+
current_compile_options += std::string(_compileOpts);
134+
}
140135
return PI_SUCCESS;
141136
}
142137

@@ -159,8 +154,8 @@ TEST(Link_Compile_Options, compile_link_Options_Test_empty_options) {
159154
current_compile_options.clear();
160155
std::string expected_options = "";
161156
static sycl::unittest::PiImage DevImage =
162-
generateEAMTestKernel1Image<EAMTestKernel1>(expected_options,
163-
expected_options);
157+
generateEAMTestKernelImage<EAMTestKernel1>(expected_options,
158+
expected_options);
164159
static sycl::unittest::PiImageArray<1> DevImageArray_{&DevImage};
165160
auto KernelID_1 = sycl::get_kernel_id<EAMTestKernel1>();
166161
sycl::queue Queue{Dev};
@@ -188,11 +183,11 @@ TEST(Link_Compile_Options, compile_link_Options_Test_filled_options) {
188183
expected_link_options_1 =
189184
"-cl-denorms-are-zero -cl-no-signed-zeros";
190185
static sycl::unittest::PiImage DevImage_1 =
191-
generateEAMTestKernel1Image<EAMTestKernel1>(expected_compile_options_1,
192-
expected_link_options_1);
186+
generateEAMTestKernelImage<EAMTestKernel2>(expected_compile_options_1,
187+
expected_link_options_1);
193188

194189
static sycl::unittest::PiImageArray<1> DevImageArray = {&DevImage_1};
195-
auto KernelID_1 = sycl::get_kernel_id<EAMTestKernel1>();
190+
auto KernelID_1 = sycl::get_kernel_id<EAMTestKernel2>();
196191
sycl::queue Queue{Dev};
197192
const sycl::context Ctx = Queue.get_context();
198193
sycl::kernel_bundle KernelBundle =
@@ -221,10 +216,10 @@ TEST(Link_Compile_Options, check_sycl_build) {
221216
std::string expected_compile_options = "-cl-opt-disable",
222217
expected_link_options = "-cl-denorms-are-zero";
223218
static sycl::unittest::PiImage DevImage =
224-
generateEAMTestKernel1Image<EAMTestKernel1>(expected_compile_options,
225-
expected_link_options);
219+
generateEAMTestKernelImage<EAMTestKernel3>(expected_compile_options,
220+
expected_link_options);
226221
static sycl::unittest::PiImageArray<1> DevImageArray{&DevImage};
227-
auto KernelID = sycl::get_kernel_id<EAMTestKernel1>();
222+
auto KernelID = sycl::get_kernel_id<EAMTestKernel3>();
228223
sycl::context Ctx{Dev};
229224
sycl::queue Queue{Ctx, Dev};
230225
sycl::kernel_bundle KernelBundle =

0 commit comments

Comments
 (0)