Skip to content

[sycl-post-link] Set isEsimdImage property for ESIMD modules after splitting #3073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

[sycl-post-link] Set isEsimdImage property for ESIMD modules after splitting #3073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cf214b
[SYCL] Added a new device binary property for ESIMD images
DenisBakhvalov Jan 20, 2021
557b564
[sycl-post-link] Set isEsimdImage property for ESIMD modules after sp…
DenisBakhvalov Jan 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions llvm/include/llvm/Support/PropertySetIO.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ class PropertySetRegistry {
"SYCL/composite specialization constants";
static constexpr char SYCL_DEVICELIB_REQ_MASK[] = "SYCL/devicelib req mask";
static constexpr char SYCL_KERNEL_PARAM_OPT_INFO[] = "SYCL/kernel param opt";
static constexpr char SYCL_IS_ESIMD_IMAGE[] = "SYCL/is ESIMD image";

// Function for bulk addition of an entire property set under given category
// (property set name).
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/Support/PropertySetIO.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ constexpr char PropertySetRegistry::SYCL_SPECIALIZATION_CONSTANTS[];
constexpr char PropertySetRegistry::SYCL_DEVICELIB_REQ_MASK[];
constexpr char PropertySetRegistry::SYCL_KERNEL_PARAM_OPT_INFO[];
constexpr char PropertySetRegistry::SYCL_COMPOSITE_SPECIALIZATION_CONSTANTS[];
constexpr char PropertySetRegistry::SYCL_IS_ESIMD_IMAGE[];

} // namespace util
} // namespace llvm
12 changes: 10 additions & 2 deletions llvm/test/tools/sycl-post-link/sycl-esimd/basic-sycl-esimd-split.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
; RUN: FileCheck %s -input-file=%t.table
; RUN: FileCheck %s -input-file=%t_0.ll --check-prefixes CHECK-SYCL-IR
; RUN: FileCheck %s -input-file=%t_esimd_0.ll --check-prefixes CHECK-ESIMD-IR
; RUN: FileCheck %s -input-file=%t_0.prop --check-prefixes CHECK-SYCL-PROP
; RUN: FileCheck %s -input-file=%t_esimd_0.prop --check-prefixes CHECK-ESIMD-PROP

; This is basic test of splitting SYCL and ESIMD kernels into separate
; modules.
; This is basic test of splitting SYCL and ESIMD kernels into separate modules.
; ESIMD module should have isEsimdImage=1 property set after splitting.

target datalayout = "e-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024"
target triple = "spir64-unknown-linux-sycldevice"
Expand Down Expand Up @@ -41,5 +43,11 @@ attributes #0 = { "sycl-module-id"="a.cpp" }
; CHECK-SYCL-IR-DAG: define dso_local spir_kernel void @SYCL_kernel()
; CHECK-SYCL-IR-DAG: declare dso_local spir_func i64 @_Z28__spirv_GlobalInvocationId_xv()

; CHECK-SYCL-PROP-NOT: [SYCL/is ESIMD image]
; CHECK-SYCL-PROP-NOT: isEsimdImage=1|1

; CHECK-ESIMD-IR-DAG: define dso_local spir_kernel void @ESIMD_kernel()
; CHECK-ESIMD-IR-DAG: declare dso_local spir_func i64 @_Z28__spirv_GlobalInvocationId_xv()

; CHECK-ESIMD-PROP: [SYCL/is ESIMD image]
; CHECK-ESIMD-PROP: isEsimdImage=1|1
7 changes: 7 additions & 0 deletions llvm/tools/sycl-post-link/sycl-post-link.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,13 @@ static string_vector saveDeviceImageProperty(
NameInfoPair.first, llvm::util::PropertyValue(Data, DataBitSize)));
}
}

if (ImgPSInfo.IsEsimdKernel) {
std::map<StringRef, bool> RMEntry = {{"isEsimdImage", true}};
PropSet.add(llvm::util::PropertySetRegistry::SYCL_IS_ESIMD_IMAGE,
RMEntry);
}
Comment on lines +590 to +594
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a whole property set for a single string, boolean pair and as I can see, this is not the first one such set:

llvm/llvm/tools/sycl-post-link/sycl-post-link.cpp

Lines 544 to 549 in 557b564

if (ImgPSInfo.NeedDeviceLibReqMask) {
uint32_t MRMask = getModuleReqMask(*ResultModules[I]);
std::map<StringRef, uint32_t> RMEntry = {{"DeviceLibReqMask", MRMask}};
PropSet.add(llvm::util::PropertySetRegistry::SYCL_DEVICELIB_REQ_MASK,
RMEntry);
}

What do you think about creating a single generic property set for boolean properties? This will slightly reduce amount of code and it will also reduce amount of temporary files created by the compiler

Tagging @kbobrovs here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexeySachkov, I don't feel qualified enough to judge your proposal. I don't know this part of the code very well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about creating a single generic property set for boolean properties?

this seems like a good idea to me with some tweaks. But I think merging this with DeviceLibReqMask would be confusing, as the device lib mask is not a boolean, but a set of booleans. So basically, SYCL/is ESIMD image could just be renamed to something like [SYCL/Binary Image Properties] which could contain all binary image-related properties, not only booleans. Having a bit mask instead of separate boolean properties wouldn't have much perf impact, but would reduce readability, IMO.

it will also reduce amount of temporary files created by the compiler

No sure about his one. @AlexeySachkov - could you elaborate please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think merging this with DeviceLibReqMask would be confusing, as the device lib mask is not a boolean, but a set of booleans.

Technically that property is absolutely the same as SYCL/is ESIMD image since each property set is a set by its nature, but for llvm::util::PropertySetRegistry::SYCL_DEVICELIB_REQ_MASK we only store a single pair within it - that's why I was referring to it as to a simple boolean property.

Note: I'm not asking to do such refactoring right now in scope of this PR, just highlighted a potential room for improvement

it will also reduce amount of temporary files created by the compiler

No sure about his one. @AlexeySachkov - could you elaborate please?

Please disregard this. By some reason I though that we emit separate .prop file for each propety set, but this is not true - we put all property set related to a particular device image and store then into a single .prop file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it would be good not to create a separate category per single property. Once out, all categories/property names become legacy which we need to maintain backward compatibility with.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe 'SYCL/misc' would also work.


std::error_code EC;
std::string SCFile =
makeResultFileName(".prop", I, ImgPSInfo.IsEsimdKernel ? "esimd_" : "");
Expand Down
2 changes: 2 additions & 0 deletions sycl/include/CL/sycl/detail/pi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,8 @@ static const uint8_t PI_DEVICE_BINARY_OFFLOAD_KIND_SYCL = 4;
#define __SYCL_PI_PROPERTY_SET_DEVICELIB_REQ_MASK "SYCL/devicelib req mask"
/// PropertySetRegistry::SYCL_KERNEL_PARAM_OPT_INFO defined in PropertySetIO.h
#define __SYCL_PI_PROPERTY_SET_KERNEL_PARAM_OPT_INFO "SYCL/kernel param opt"
/// PropertySetRegistry::SYCL_IS_ESIMD_IMAGE defined in PropertySetIO.h
#define __SYCL_PI_PROPERTY_SET_SYCL_IS_ESIMD_IMAGE "SYCL/is ESIMD image"

/// This struct is a record of the device binary information. If the Kind field
/// denotes a portable binary type (SPIR-V or LLVM IR), the DeviceTargetSpec
Expand Down
2 changes: 2 additions & 0 deletions sycl/include/CL/sycl/detail/pi.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ class DeviceBinaryImage {
const PropertyRange &getKernelParamOptInfo() const {
return KernelParamOptInfo;
}
const PropertyRange &getSyclIsEsimdImage() const { return SyclIsEsimdImage; }
virtual ~DeviceBinaryImage() {}

protected:
Expand All @@ -332,6 +333,7 @@ class DeviceBinaryImage {
DeviceBinaryImage::PropertyRange CompositeSpecConstIDMap;
DeviceBinaryImage::PropertyRange DeviceLibReqMask;
DeviceBinaryImage::PropertyRange KernelParamOptInfo;
DeviceBinaryImage::PropertyRange SyclIsEsimdImage;
};

/// Tries to determine the device binary image foramat. Returns
Expand Down
1 change: 1 addition & 0 deletions sycl/source/detail/pi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,7 @@ void DeviceBinaryImage::init(pi_device_binary Bin) {
__SYCL_PI_PROPERTY_SET_COMPOSITE_SPEC_CONST_MAP);
DeviceLibReqMask.init(Bin, __SYCL_PI_PROPERTY_SET_DEVICELIB_REQ_MASK);
KernelParamOptInfo.init(Bin, __SYCL_PI_PROPERTY_SET_KERNEL_PARAM_OPT_INFO);
SyclIsEsimdImage.init(Bin, __SYCL_PI_PROPERTY_SET_SYCL_IS_ESIMD_IMAGE);
}

} // namespace pi
Expand Down