[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.

Author: maksimsab

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 <istream>
-#include <map>
-#include <memory>
-#include <string>
-#include <type_traits>
+#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<StringRef, PropertyValue>;
+/// 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<SmallString<16>, unsigned, PropertySetKeyInfo>;
+/// A property set. Preserves insertion order when iterating elements.
+using PropertySet = MapVector<SmallString<16>, 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<StringRef, PropertySet>;
+  using MapTy = MapVector<SmallString<16>, 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 <typename MapTy> void add(StringRef Category, const MapTy &Props) {
-    using KeyTy = typename MapTy::value_type::first_type;
-    static_assert(std::is_same<typename std::remove_const<KeyTy>::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 <typename T>
   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<std::unique_ptr<PropertySetRegistry>>
   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:

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";
     }
   }
 }

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

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}

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,16 +443,15 @@ std::string saveModuleProperties(module_split::ModuleDesc &MD,
   SmallVector<std::string, 4> 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<uint32_t> KernelReqdWorkGroupSize =
           getKernelReqdWorkGroupSizeMetadata(Func);
       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<StringRef> 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);

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]]