From 43201db155d7ac67f89d858b5b9d7217e25dbf9c Mon Sep 17 00:00:00 2001 From: Maksim Sabianin Date: Fri, 8 Mar 2024 15:49:08 +0100 Subject: [PATCH] [SYCL] Make PropertySetRegistry being owning it's content (#12582) Before this patch PropertySetRegistry used StringRefs that point at external data that was supposed to outlive the instance of PropertySetRegistry. It wasn't obvious and it might lead to memory bugs. This patch makes PropertySetRegistry owning it's content as containers usually do. Also documentation is aligned with doxygen BKSM. --- llvm/include/llvm/Support/PropertySetIO.h | 56 ++++++++++--------- llvm/lib/Support/PropertySetIO.cpp | 2 +- .../spec-constants/SYCL-2020.ll | 19 ++++--- .../SYCL2020-struct-with-undef-padding.ll | 1 + llvm/tools/sycl-post-link/sycl-post-link.cpp | 29 +++++----- .../SYCL-2020-spec-const-ids-order.cpp | 2 +- 6 files changed, 57 insertions(+), 52 deletions(-) diff --git a/llvm/include/llvm/Support/PropertySetIO.h b/llvm/include/llvm/Support/PropertySetIO.h index cb257ba7e5c77..93e045256ed93 100644 --- a/llvm/include/llvm/Support/PropertySetIO.h +++ b/llvm/include/llvm/Support/PropertySetIO.h @@ -34,16 +34,12 @@ #define LLVM_SUPPORT_PROPERTYSETIO_H #include "llvm/ADT/MapVector.h" +#include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Error.h" #include "llvm/Support/MemoryBuffer.h" #include "llvm/Support/raw_ostream.h" - -#include -#include -#include -#include -#include +#include "llvm/Support/xxhash.h" namespace llvm { namespace util { @@ -175,14 +171,28 @@ class PropertyValue { } Val; }; -// A property set. Preserves insertion order when iterating elements. -using PropertySet = MapVector; +/// Structure for specialization of DenseMap in PropertySetRegistry. +struct PropertySetKeyInfo { + static unsigned getHashValue(const SmallString<16> &K) { return xxHash64(K); } + + static SmallString<16> getEmptyKey() { return SmallString<16>(""); } -// A "registry" of multiple property sets. Maps a property set name to its -// contents. Can be read/written. + static SmallString<16> getTombstoneKey() { return SmallString<16>("_"); } + + static bool isEqual(StringRef L, StringRef R) { return L == R; } +}; + +using PropertyMapTy = DenseMap, unsigned, PropertySetKeyInfo>; +/// A property set. Preserves insertion order when iterating elements. +using PropertySet = MapVector, PropertyValue, PropertyMapTy>; + +/// A registry of property sets. Maps a property set name to its +/// content. +/// +/// The order of keys is preserved and corresponds to the order of insertion. class PropertySetRegistry { public: - using MapTy = MapVector; + using MapTy = MapVector, PropertySet, PropertyMapTy>; // Specific property category names used by tools. static constexpr char SYCL_SPECIALIZATION_CONSTANTS[] = @@ -199,44 +209,38 @@ class PropertySetRegistry { static constexpr char SYCL_DEVICE_REQUIREMENTS[] = "SYCL/device requirements"; static constexpr char SYCL_HOST_PIPES[] = "SYCL/host pipes"; - // Function for bulk addition of an entire property set under given category - // (property set name). + /// Function for bulk addition of an entire property set in the given + /// \p Category . template void add(StringRef Category, const MapTy &Props) { - using KeyTy = typename MapTy::value_type::first_type; - static_assert(std::is_same::type, - llvm::StringRef>::value, - "wrong key type"); - assert(PropSetMap.find(Category) == PropSetMap.end() && "category already added"); auto &PropSet = PropSetMap[Category]; for (const auto &Prop : Props) - PropSet.insert({Prop.first, PropertyValue(Prop.second)}); + PropSet.insert_or_assign(Prop.first, PropertyValue(Prop.second)); } - // Function to add a property to a given category (property set name). + /// Adds the given \p PropVal with the given \p PropName into the given \p + /// Category . template void add(StringRef Category, StringRef PropName, const T &PropVal) { auto &PropSet = PropSetMap[Category]; PropSet.insert({PropName, PropertyValue(PropVal)}); } - // Parses and creates a property set registry. + /// Parses from the given \p Buf a property set registry. static Expected> read(const MemoryBuffer *Buf); - // Dumps a property set registry to a stream. + /// Dumps the property set registry to the given \p Out stream. void write(raw_ostream &Out) const; - // Start iterator of all preperty sets in the registry. MapTy::const_iterator begin() const { return PropSetMap.begin(); } - // End iterator of all preperty sets in the registry. MapTy::const_iterator end() const { return PropSetMap.end(); } - // Retrieves a property set with given name. + /// Retrieves a property set with given \p Name . PropertySet &operator[](StringRef Name) { return PropSetMap[Name]; } - // Constant access to the underlying map. + /// Constant access to the underlying map. const MapTy &getPropSets() const { return PropSetMap; } private: diff --git a/llvm/lib/Support/PropertySetIO.cpp b/llvm/lib/Support/PropertySetIO.cpp index ffb6394913fc9..96593d4aa26be 100644 --- a/llvm/lib/Support/PropertySetIO.cpp +++ b/llvm/lib/Support/PropertySetIO.cpp @@ -126,7 +126,7 @@ void PropertySetRegistry::write(raw_ostream &Out) const { Out << "[" << PropSet.first << "]\n"; for (const auto &Props : PropSet.second) { - Out << std::string(Props.first) << "=" << Props.second << "\n"; + Out << Props.first << "=" << Props.second << "\n"; } } } diff --git a/llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll b/llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll index daa0a08ec9231..113479edc448a 100644 --- a/llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll +++ b/llvm/test/tools/sycl-post-link/spec-constants/SYCL-2020.ll @@ -268,15 +268,16 @@ attributes #3 = { nounwind } ; CHECK-RT-SAME: i32 [[#SCID17]], i32 4, i32 4} ; CHECK-PROPS: [SYCL/specialization constants] -; CHECK-PROPS: _ZTS14name_generatorIL_Z9id_halfEE=2| -; CHECK-PROPS: _ZTS14name_generatorIL_Z6id_intEE=2| -; CHECK-PROPS: _ZTS14name_generatorIL_Z9id_composEE=2| -; CHECK-PROPS: _ZTS14name_generatorIL_Z10id_compos2EE=2| -; CHECK-PROPS: _ZTS14name_generatorIL_Z10id_vectorEE=2| -; CHECK-PROPS: _ZTS14name_generatorIL_Z10id_marrayEE=2| -; CHECK-PROPS: _ZTS14name_generatorIL_Z10id_marray2EE=2| -; CHECK-PROPS: _ZTS14name_generatorIL_Z10id_marray3EE=2| -; CHECK-PROPS: _ZTS14name_generatorIL_Z10id_marray4EE=2| +; CHECK-PROPS-NEXT: _ZTS14name_generatorIL_Z9id_halfEE=2| +; CHECK-PROPS-NEXT: _ZTS14name_generatorIL_Z6id_intEE=2| +; CHECK-PROPS-NEXT: _ZTS14name_generatorIL_Z9id_composEE=2| +; CHECK-PROPS-NEXT: _ZTS14name_generatorIL_Z10id_compos2EE=2| +; CHECK-PROPS-NEXT: _ZTS14name_generatorIL_Z10id_compos3EE=2| +; CHECK-PROPS-NEXT: _ZTS14name_generatorIL_Z10id_vectorEE=2| +; CHECK-PROPS-NEXT: _ZTS14name_generatorIL_Z10id_marrayEE=2| +; CHECK-PROPS-NEXT: _ZTS14name_generatorIL_Z10id_marray2EE=2| +; CHECK-PROPS-NEXT: _ZTS14name_generatorIL_Z10id_marray3EE=2| +; CHECK-PROPS-NEXT: _ZTS14name_generatorIL_Z10id_marray4EE=2| ; CHECK-PROPS-DEF: [SYCL/specialization constants default values] ; CHECK-PROPS-DEF: all=2| ; CHECK-LOG: sycl.specialization-constants diff --git a/llvm/test/tools/sycl-post-link/spec-constants/SYCL2020-struct-with-undef-padding.ll b/llvm/test/tools/sycl-post-link/spec-constants/SYCL2020-struct-with-undef-padding.ll index 21fbdca449102..1511c68a7e381 100644 --- a/llvm/test/tools/sycl-post-link/spec-constants/SYCL2020-struct-with-undef-padding.ll +++ b/llvm/test/tools/sycl-post-link/spec-constants/SYCL2020-struct-with-undef-padding.ll @@ -85,6 +85,7 @@ attributes #4 = { convergent } ; CHECK-PROP: [SYCL/specialization constants] ; CHECK-PROP-NEXT: ef880fa09cf7a9d7____ZL8coeff_id=2| +; CHECK-PROP-NEXT: df991fa0adf9bad8____ZL8coeff_id2=2| ; CHECK-LOG: sycl.specialization-constants ; CHECK-LOG:[[UNIQUE_PREFIX:[0-9a-zA-Z]+]]={0, 0, 4} ; CHECK-LOG:[[UNIQUE_PREFIX]]={1, 4, 4} diff --git a/llvm/tools/sycl-post-link/sycl-post-link.cpp b/llvm/tools/sycl-post-link/sycl-post-link.cpp index ff27b31a0499e..3040270aea540 100644 --- a/llvm/tools/sycl-post-link/sycl-post-link.cpp +++ b/llvm/tools/sycl-post-link/sycl-post-link.cpp @@ -434,8 +434,7 @@ std::string saveModuleProperties(module_split::ModuleDesc &MD, // so they won't make it into the export list. Should the check be // F->getCallingConv() != CallingConv::SPIR_KERNEL? if (F->getCallingConv() == CallingConv::SPIR_FUNC) { - PropSet[PropSetRegTy::SYCL_EXPORTED_SYMBOLS].insert( - {F->getName(), true}); + PropSet.add(PropSetRegTy::SYCL_EXPORTED_SYMBOLS, F->getName(), true); } } } @@ -444,8 +443,6 @@ std::string saveModuleProperties(module_split::ModuleDesc &MD, SmallVector MetadataNames; if (GlobProps.EmitProgramMetadata) { - auto &ProgramMetadata = PropSet[PropSetRegTy::SYCL_PROGRAM_METADATA]; - // Add reqd_work_group_size information to program metadata for (const Function &Func : M.functions()) { std::vector KernelReqdWorkGroupSize = @@ -453,7 +450,8 @@ std::string saveModuleProperties(module_split::ModuleDesc &MD, if (KernelReqdWorkGroupSize.empty()) continue; MetadataNames.push_back(Func.getName().str() + "@reqd_work_group_size"); - ProgramMetadata.insert({MetadataNames.back(), KernelReqdWorkGroupSize}); + PropSet.add(PropSetRegTy::SYCL_PROGRAM_METADATA, MetadataNames.back(), + KernelReqdWorkGroupSize); } // Add global_id_mapping information with mapping between device-global @@ -464,11 +462,12 @@ std::string saveModuleProperties(module_split::ModuleDesc &MD, StringRef GlobalID = getGlobalVariableUniqueId(GV); MetadataNames.push_back(GlobalID.str() + "@global_id_mapping"); - ProgramMetadata.insert({MetadataNames.back(), GV.getName()}); + PropSet.add(PropSetRegTy::SYCL_PROGRAM_METADATA, MetadataNames.back(), + GV.getName()); } } if (MD.isESIMD()) { - PropSet[PropSetRegTy::SYCL_MISC_PROP].insert({"isEsimdImage", true}); + PropSet.add(PropSetRegTy::SYCL_MISC_PROP, "isEsimdImage", true); } { StringRef RegAllocModeAttr = "sycl-register-alloc-mode"; @@ -482,8 +481,8 @@ std::string saveModuleProperties(module_split::ModuleDesc &MD, return true; }); if (HasRegAllocMode) { - PropSet[PropSetRegTy::SYCL_MISC_PROP].insert( - {RegAllocModeAttr, RegAllocModeVal}); + PropSet.add(PropSetRegTy::SYCL_MISC_PROP, RegAllocModeAttr, + RegAllocModeVal); } } @@ -499,7 +498,7 @@ std::string saveModuleProperties(module_split::ModuleDesc &MD, return true; }); if (HasGRFSize) { - PropSet[PropSetRegTy::SYCL_MISC_PROP].insert({GRFSizeAttr, GRFSizeVal}); + PropSet.add(PropSetRegTy::SYCL_MISC_PROP, GRFSizeAttr, GRFSizeVal); } } @@ -534,17 +533,17 @@ std::string saveModuleProperties(module_split::ModuleDesc &MD, } if (OptLevel != -1) - PropSet[PropSetRegTy::SYCL_MISC_PROP].insert({"optLevel", OptLevel}); + PropSet.add(PropSetRegTy::SYCL_MISC_PROP, "optLevel", OptLevel); } { std::vector FuncNames = getKernelNamesUsingAssert(M); for (const StringRef &FName : FuncNames) - PropSet[PropSetRegTy::SYCL_ASSERT_USED].insert({FName, true}); + PropSet.add(PropSetRegTy::SYCL_ASSERT_USED, FName, true); } { if (isModuleUsingAsan(M)) - PropSet[PropSetRegTy::SYCL_MISC_PROP].insert({"asanUsed", true}); + PropSet.add(PropSetRegTy::SYCL_MISC_PROP, "asanUsed", true); } if (GlobProps.EmitDeviceGlobalPropSet) { @@ -560,8 +559,8 @@ std::string saveModuleProperties(module_split::ModuleDesc &MD, } if (MD.isSpecConstantDefault()) - PropSet[PropSetRegTy::SYCL_MISC_PROP].insert( - {"specConstsReplacedWithDefault", 1}); + PropSet.add(PropSetRegTy::SYCL_MISC_PROP, "specConstsReplacedWithDefault", + 1); std::error_code EC; std::string SCFile = makeResultFileName(".prop", I, Suff); diff --git a/sycl/test/basic_tests/SYCL-2020-spec-const-ids-order.cpp b/sycl/test/basic_tests/SYCL-2020-spec-const-ids-order.cpp index 762d73983db32..0aefe26bd826c 100644 --- a/sycl/test/basic_tests/SYCL-2020-spec-const-ids-order.cpp +++ b/sycl/test/basic_tests/SYCL-2020-spec-const-ids-order.cpp @@ -50,7 +50,7 @@ int main() { // CHECK-PROP-NEXT: [[UNIQUE_PREFIX]]____ZL10ConstantId // CHECK-PROP-NEXT: [[UNIQUE_PREFIX]]____ZL11SecondValue // CHECK-PROP-NEXT: [[UNIQUE_PREFIX]]____ZL11SpecConst42 -// + // CHECK-IR: !sycl.specialization-constants = !{![[#MD0:]], ![[#MD1:]], ![[#MD2:]], ![[#MD3:]]} // CHECK-IR: ![[#MD0]] = !{!"[[UNIQUE_PREFIX:[a-z0-9]+]]____ZL5Val23", i32 [[#ID:]] // CHECK-IR: ![[#MD1]] = !{!"[[UNIQUE_PREFIX]]____ZL10ConstantId", i32 [[#ID+1]]