From ccf64e58d54079f6a2a1be63525d2cb86f4013d1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A1bio=20Mestre?= <fabio.mestre@codeplay.com>
Date: Mon, 27 Jan 2025 14:15:16 +0000
Subject: [PATCH 1/5] Add common reference semantics to sycl graphs

Adds missing common reference semantic functionality such
as operator==, operator!= and hash functions to all
sycl graph related classes.
---
 .../sycl/ext/oneapi/experimental/graph.hpp    |  84 +++++++-
 sycl/source/detail/graph_impl.cpp             |  23 ++-
 sycl/source/detail/graph_impl.hpp             |  28 ++-
 .../Extensions/CommandGraph/CMakeLists.txt    |   1 +
 .../CommandGraph/CommonReferenceSemantics.cpp | 193 ++++++++++++++++++
 5 files changed, 323 insertions(+), 6 deletions(-)
 create mode 100644 sycl/unittests/Extensions/CommandGraph/CommonReferenceSemantics.cpp

diff --git a/sycl/include/sycl/ext/oneapi/experimental/graph.hpp b/sycl/include/sycl/ext/oneapi/experimental/graph.hpp
index e2e87c30ea945..ac0e713dfde95 100644
--- a/sycl/include/sycl/ext/oneapi/experimental/graph.hpp
+++ b/sycl/include/sycl/ext/oneapi/experimental/graph.hpp
@@ -17,7 +17,7 @@
 #ifdef __INTEL_PREVIEW_BREAKING_CHANGES
 #include <sycl/detail/string_view.hpp>
 #endif
-#include <sycl/device.hpp>                     // for device
+#include <sycl/device.hpp> // for device
 #include <sycl/ext/oneapi/experimental/detail/properties/graph_properties.hpp> // for graph properties classes
 #include <sycl/nd_range.hpp>                   // for range, nd_range
 #include <sycl/properties/property_traits.hpp> // for is_property, is_property_of
@@ -142,6 +142,14 @@ class __SYCL_EXPORT node {
   /// Update the Range of this node if it is a kernel execution node
   template <int Dimensions> void update_range(range<Dimensions> executionRange);
 
+  /// Common Reference Semantics
+  friend bool operator==(const node &LHS, const node &RHS) {
+    return LHS.impl == RHS.impl;
+  }
+  friend bool operator!=(const node &LHS, const node &RHS) {
+    return LHS.impl != RHS.impl;
+  }
+
 private:
   node(const std::shared_ptr<detail::node_impl> &Impl) : impl(Impl) {}
 
@@ -181,6 +189,16 @@ class __SYCL_EXPORT dynamic_command_group {
   size_t get_active_index() const;
   void set_active_index(size_t Index);
 
+  /// Common Reference Semantics
+  friend bool operator==(const dynamic_command_group &LHS,
+                         const dynamic_command_group &RHS) {
+    return LHS.impl == RHS.impl;
+  }
+  friend bool operator!=(const dynamic_command_group &LHS,
+                         const dynamic_command_group &RHS) {
+    return LHS.impl != RHS.impl;
+  }
+
 private:
   template <class Obj>
   friend const decltype(Obj::impl) &
@@ -307,6 +325,16 @@ class __SYCL_EXPORT modifiable_command_graph
   /// Get a list of all root nodes (nodes without dependencies) in this graph.
   std::vector<node> get_root_nodes() const;
 
+  /// Common Reference Semantics
+  friend bool operator==(const modifiable_command_graph &LHS,
+                         const modifiable_command_graph &RHS) {
+    return LHS.impl == RHS.impl;
+  }
+  friend bool operator!=(const modifiable_command_graph &LHS,
+                         const modifiable_command_graph &RHS) {
+    return LHS.impl != RHS.impl;
+  }
+
 protected:
   /// Constructor used internally by the runtime.
   /// @param Impl Detail implementation class to construct object with.
@@ -386,6 +414,16 @@ class __SYCL_EXPORT executable_command_graph
   /// @param Nodes The nodes to use for updating the graph.
   void update(const std::vector<node> &Nodes);
 
+  /// Common Reference Semantics
+  friend bool operator==(const executable_command_graph &LHS,
+                         const executable_command_graph &RHS) {
+    return LHS.impl == RHS.impl;
+  }
+  friend bool operator!=(const executable_command_graph &LHS,
+                         const executable_command_graph &RHS) {
+    return LHS.impl != RHS.impl;
+  }
+
 protected:
   /// Constructor used by internal runtime.
   /// @param Graph Detail implementation class to construct with.
@@ -452,6 +490,16 @@ class __SYCL_EXPORT dynamic_parameter_base {
           Graph,
       size_t ParamSize, const void *Data);
 
+  /// Common Reference Semantics
+  friend bool operator==(const dynamic_parameter_base &LHS,
+                         const dynamic_parameter_base &RHS) {
+    return LHS.impl == RHS.impl;
+  }
+  friend bool operator!=(const dynamic_parameter_base &LHS,
+                         const dynamic_parameter_base &RHS) {
+    return LHS.impl != RHS.impl;
+  }
+
 protected:
   void updateValue(const void *NewValue, size_t Size);
 
@@ -512,3 +560,37 @@ command_graph(const context &SyclContext, const device &SyclDevice,
 
 } // namespace _V1
 } // namespace sycl
+
+namespace std {
+template <> struct __SYCL_EXPORT hash<sycl::ext::oneapi::experimental::node> {
+  size_t operator()(const sycl::ext::oneapi::experimental::node &Node) const;
+};
+
+template <>
+struct __SYCL_EXPORT
+    hash<sycl::ext::oneapi::experimental::dynamic_command_group> {
+  size_t operator()(const sycl::ext::oneapi::experimental::dynamic_command_group
+                        &DynamicCGH) const;
+};
+
+template <sycl::ext::oneapi::experimental::graph_state State>
+struct __SYCL_EXPORT
+    hash<sycl::ext::oneapi::experimental::command_graph<State>> {
+  size_t operator()(const sycl::ext::oneapi::experimental::command_graph<State>
+                        &Graph) const {
+    auto ID = sycl::detail::getSyclObjImpl(Graph)->getID();
+    return std::hash<decltype(ID)>()(ID);
+  }
+};
+
+template <typename ValueT>
+struct __SYCL_EXPORT
+    hash<sycl::ext::oneapi::experimental::dynamic_parameter<ValueT>> {
+  size_t
+  operator()(const sycl::ext::oneapi::experimental::dynamic_parameter<ValueT>
+                 &DynamicParam) const {
+    auto ID = sycl::detail::getSyclObjImpl(DynamicParam)->getID();
+    return std::hash<decltype(ID)>()(ID);
+  }
+};
+} // namespace std
diff --git a/sycl/source/detail/graph_impl.cpp b/sycl/source/detail/graph_impl.cpp
index e6181a559d8e6..a498cc455bd85 100644
--- a/sycl/source/detail/graph_impl.cpp
+++ b/sycl/source/detail/graph_impl.cpp
@@ -324,7 +324,8 @@ graph_impl::graph_impl(const sycl::context &SyclContext,
                        const sycl::device &SyclDevice,
                        const sycl::property_list &PropList)
     : MContext(SyclContext), MDevice(SyclDevice), MRecordingQueues(),
-      MEventsMap(), MInorderQueueMap() {
+      MEventsMap(), MInorderQueueMap(),
+      MID(NextAvailableID.fetch_add(1, std::memory_order_relaxed)) {
   checkGraphPropertiesAndThrow(PropList);
   if (PropList.has_property<property::graph::no_cycle_check>()) {
     MSkipCycleChecks = true;
@@ -913,7 +914,8 @@ exec_graph_impl::exec_graph_impl(sycl::context Context,
       MExecutionEvents(),
       MIsUpdatable(PropList.has_property<property::graph::updatable>()),
       MEnableProfiling(
-          PropList.has_property<property::graph::enable_profiling>()) {
+          PropList.has_property<property::graph::enable_profiling>()),
+      MID(NextAvailableID.fetch_add(1, std::memory_order_relaxed)) {
   checkGraphPropertiesAndThrow(PropList);
   // If the graph has been marked as updatable then check if the backend
   // actually supports that. Devices supporting aspect::ext_oneapi_graph must
@@ -2035,7 +2037,8 @@ void dynamic_parameter_impl::updateCGAccessor(
 
 dynamic_command_group_impl::dynamic_command_group_impl(
     const command_graph<graph_state::modifiable> &Graph)
-    : MGraph{sycl::detail::getSyclObjImpl(Graph)}, MActiveCGF(0) {}
+    : MGraph{sycl::detail::getSyclObjImpl(Graph)}, MActiveCGF(0),
+      MID(NextAvailableID.fetch_add(1, std::memory_order_relaxed)) {}
 
 void dynamic_command_group_impl::finalizeCGFList(
     const std::vector<std::function<void(handler &)>> &CGFList) {
@@ -2159,3 +2162,17 @@ void dynamic_command_group::set_active_index(size_t Index) {
 } // namespace ext
 } // namespace _V1
 } // namespace sycl
+
+size_t std::hash<sycl::ext::oneapi::experimental::node>::operator()(
+    const sycl::ext::oneapi::experimental::node &Node) const {
+  auto ID = sycl::detail::getSyclObjImpl(Node)->getID();
+  return std::hash<decltype(ID)>()(ID);
+}
+
+size_t
+std::hash<sycl::ext::oneapi::experimental::dynamic_command_group>::operator()(
+    const sycl::ext::oneapi::experimental::dynamic_command_group &DynamicCGH)
+    const {
+  auto ID = sycl::detail::getSyclObjImpl(DynamicCGH)->getID();
+  return std::hash<decltype(ID)>()(ID);
+}
diff --git a/sycl/source/detail/graph_impl.hpp b/sycl/source/detail/graph_impl.hpp
index e609123b4f285..114b50a81d38e 100644
--- a/sycl/source/detail/graph_impl.hpp
+++ b/sycl/source/detail/graph_impl.hpp
@@ -1120,6 +1120,8 @@ class graph_impl : public std::enable_shared_from_this<graph_impl> {
     return MBarrierDependencyMap[Queue];
   }
 
+  unsigned long long getID() { return MID; }
+
 private:
   /// Iterate over the graph depth-first and run \p NodeFunc on each node.
   /// @param NodeFunc A function which receives as input a node in the graph to
@@ -1198,6 +1200,9 @@ class graph_impl : public std::enable_shared_from_this<graph_impl> {
   std::map<std::weak_ptr<sycl::detail::queue_impl>, std::shared_ptr<node_impl>,
            std::owner_less<std::weak_ptr<sycl::detail::queue_impl>>>
       MBarrierDependencyMap;
+
+  unsigned long long MID;
+  inline static std::atomic<unsigned long long> NextAvailableID = 0;
 };
 
 /// Class representing the implementation of command_graph<executable>.
@@ -1297,6 +1302,8 @@ class exec_graph_impl {
 
   void updateImpl(std::shared_ptr<node_impl> NodeImpl);
 
+  unsigned long long getID() { return MID; }
+
 private:
   /// Create a command-group for the node and add it to command-buffer by going
   /// through the scheduler.
@@ -1408,13 +1415,17 @@ class exec_graph_impl {
   // Stores a cache of node ids from modifiable graph nodes to the companion
   // node(s) in this graph. Used for quick access when updating this graph.
   std::multimap<node_impl::id_type, std::shared_ptr<node_impl>> MIDCache;
+
+  unsigned long long MID;
+  inline static std::atomic<unsigned long long> NextAvailableID = 0;
 };
 
 class dynamic_parameter_impl {
 public:
   dynamic_parameter_impl(std::shared_ptr<graph_impl> GraphImpl,
                          size_t ParamSize, const void *Data)
-      : MGraph(GraphImpl), MValueStorage(ParamSize) {
+      : MGraph(GraphImpl), MValueStorage(ParamSize),
+        MID(NextAvailableID.fetch_add(1, std::memory_order_relaxed)) {
     std::memcpy(MValueStorage.data(), Data, ParamSize);
   }
 
@@ -1422,7 +1433,8 @@ class dynamic_parameter_impl {
   /// Parameter size is taken from member of raw_kernel_arg object.
   dynamic_parameter_impl(std::shared_ptr<graph_impl> GraphImpl, size_t,
                          raw_kernel_arg *Data)
-      : MGraph(GraphImpl) {
+      : MGraph(GraphImpl),
+        MID(NextAvailableID.fetch_add(1, std::memory_order_relaxed)) {
     size_t RawArgSize = Data->MArgSize;
     const void *RawArgData = Data->MArgData;
     MValueStorage.reserve(RawArgSize);
@@ -1493,6 +1505,8 @@ class dynamic_parameter_impl {
                                int ArgIndex,
                                const sycl::detail::AccessorBaseHost *Acc);
 
+  unsigned long long getID() { return MID; }
+
   // Weak ptrs to node_impls which will be updated
   std::vector<std::pair<std::weak_ptr<node_impl>, int>> MNodes;
   // Dynamic command-groups which will be updated
@@ -1500,6 +1514,10 @@ class dynamic_parameter_impl {
 
   std::shared_ptr<graph_impl> MGraph;
   std::vector<std::byte> MValueStorage;
+
+private:
+  unsigned long long MID;
+  inline static std::atomic<unsigned long long> NextAvailableID = 0;
 };
 
 class dynamic_command_group_impl
@@ -1540,6 +1558,12 @@ class dynamic_command_group_impl
 
   /// List of nodes using this dynamic command-group.
   std::vector<std::weak_ptr<node_impl>> MNodes;
+
+  unsigned long long getID() { return MID; }
+
+private:
+  unsigned long long MID;
+  inline static std::atomic<unsigned long long> NextAvailableID = 0;
 };
 } // namespace detail
 } // namespace experimental
diff --git a/sycl/unittests/Extensions/CommandGraph/CMakeLists.txt b/sycl/unittests/Extensions/CommandGraph/CMakeLists.txt
index 31f899f6a2349..a06adf5113277 100644
--- a/sycl/unittests/Extensions/CommandGraph/CMakeLists.txt
+++ b/sycl/unittests/Extensions/CommandGraph/CMakeLists.txt
@@ -3,6 +3,7 @@ set(CMAKE_CXX_EXTENSIONS OFF)
 add_sycl_unittest(CommandGraphExtensionTests OBJECT
   Barrier.cpp
   CommandGraph.cpp
+  CommonReferenceSemantics.cpp
   Exceptions.cpp
   InOrderQueue.cpp
   MultiThreaded.cpp
diff --git a/sycl/unittests/Extensions/CommandGraph/CommonReferenceSemantics.cpp b/sycl/unittests/Extensions/CommandGraph/CommonReferenceSemantics.cpp
new file mode 100644
index 0000000000000..ee23782c03093
--- /dev/null
+++ b/sycl/unittests/Extensions/CommandGraph/CommonReferenceSemantics.cpp
@@ -0,0 +1,193 @@
+//==----------------------- CommandGraph.cpp -------------------------------==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#include "Common.hpp"
+
+#include <unordered_map>
+
+using namespace sycl;
+using namespace sycl::ext::oneapi;
+
+/**
+ * Checks that the operators and constructors of graph related classes meet the
+ * common reference semantics.
+ * @param objFactory A function object that returns an object to be tested.
+ */
+template <typename T, typename LambdaType>
+void testSemantics(LambdaType &&objFactory) {
+
+  T Obj1 = objFactory();
+  T Obj2 = objFactory();
+
+  // Check the == and != operators.
+  ASSERT_FALSE(Obj1 == Obj2);
+  ASSERT_TRUE(Obj1 != Obj2);
+
+  // Check Copy Constructor and Assignment operators.
+  T Obj1Copy = Obj1;
+  T Obj2CopyConstructed(Obj2);
+  ASSERT_TRUE(Obj1Copy == Obj1);
+  ASSERT_TRUE(Obj2CopyConstructed == Obj2);
+
+  // Check Move Constructor and Move Assignment operators.
+  auto Obj1Move = std::move(Obj1);
+  auto Obj2MoveConstructed(std::move(Obj2));
+  ASSERT_TRUE(Obj1Move == Obj1Copy);
+  ASSERT_TRUE(Obj2MoveConstructed == Obj2CopyConstructed);
+}
+
+TEST_F(CommandGraphTest, ModifiableGraphSemantics) {
+  sycl::queue Queue;
+  auto factory = [&]() {
+    return experimental::command_graph(Queue.get_context(), Queue.get_device());
+  };
+
+  ASSERT_NO_FATAL_FAILURE(
+      testSemantics<
+          experimental::command_graph<experimental::graph_state::modifiable>>(
+          factory));
+}
+
+TEST_F(CommandGraphTest, ExecutableGraphSemantics) {
+  sycl::queue Queue;
+
+  auto factory = [&]() {
+    experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
+    return Graph.finalize();
+  };
+  ASSERT_NO_FATAL_FAILURE(
+      testSemantics<
+          experimental::command_graph<experimental::graph_state::executable>>(
+          factory));
+}
+
+TEST_F(CommandGraphTest, NodeSemantics) {
+  sycl::queue Queue;
+  experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
+
+  auto factory = [&]() {
+    return Graph.add(
+        [&](handler &CGH) { CGH.parallel_for(1, [=](item<1> Item) {}); });
+  };
+  ASSERT_NO_FATAL_FAILURE(testSemantics<experimental::node>(factory));
+}
+
+TEST_F(CommandGraphTest, DynamicCGSemantics) {
+  sycl::queue Queue;
+  experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
+
+  auto CGF = [&](handler &CGH) { CGH.parallel_for(1, [=](item<1> Item) {}); };
+
+  auto factory = [&]() {
+    return experimental::dynamic_command_group(Graph, {CGF});
+  };
+  ASSERT_NO_FATAL_FAILURE(
+      testSemantics<experimental::dynamic_command_group>(factory));
+}
+
+TEST_F(CommandGraphTest, DynamicParamSemantics) {
+  sycl::queue Queue;
+  experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
+
+  auto factory = [&]() {
+    return experimental::dynamic_parameter<int>(Graph, 1);
+  };
+  ASSERT_NO_FATAL_FAILURE(
+      testSemantics<experimental::dynamic_parameter<int>>(factory));
+}
+
+/**
+ * Checks for potential hash collisions in the hash implementations of graph
+ * related classes.
+ * @param objFactory A function object that returns an object to be tested.
+ */
+template <typename T, typename LambdaType>
+void testHash(LambdaType &&objFactory) {
+
+  const int NumObjects = 100;
+
+  std::unordered_map<T, bool> MapObjToBool{};
+  T Obj1 = objFactory();
+  T Obj2 = objFactory();
+  T Obj3 = objFactory();
+  T Obj4 = objFactory();
+
+  ASSERT_TRUE(MapObjToBool.insert({Obj1, true}).second);
+  ASSERT_TRUE(MapObjToBool.insert({Obj2, true}).second);
+
+  // Insert objects and destroy them immediately to confirm that this doesn't
+  // create collisions with later insertions.
+  for (int i = 0; i < NumObjects; ++i) {
+    T ObjI = objFactory();
+    ASSERT_TRUE(MapObjToBool.insert({ObjI, true}).second);
+  }
+
+  ASSERT_TRUE(MapObjToBool.insert({Obj3, true}).second);
+  ASSERT_TRUE(MapObjToBool.insert({Obj4, true}).second);
+
+  ASSERT_TRUE(MapObjToBool.size() == (NumObjects + 4));
+}
+
+TEST_F(CommandGraphTest, ModifiableGraphHash) {
+  sycl::queue Queue;
+  auto factory = [&]() {
+    return experimental::command_graph(Queue.get_context(), Queue.get_device());
+  };
+
+  ASSERT_NO_FATAL_FAILURE(
+      testHash<
+          experimental::command_graph<experimental::graph_state::modifiable>>(
+          factory));
+}
+
+TEST_F(CommandGraphTest, ExecutableGraphHash) {
+  sycl::queue Queue;
+
+  auto factory = [&]() {
+    experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
+    return Graph.finalize();
+  };
+  ASSERT_NO_FATAL_FAILURE(
+      testHash<
+          experimental::command_graph<experimental::graph_state::executable>>(
+          factory));
+}
+
+TEST_F(CommandGraphTest, NodeHash) {
+  sycl::queue Queue;
+  experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
+
+  auto factory = [&]() {
+    return Graph.add(
+        [&](handler &CGH) { CGH.parallel_for(1, [=](item<1> Item) {}); });
+  };
+  ASSERT_NO_FATAL_FAILURE(testHash<experimental::node>(factory));
+}
+
+TEST_F(CommandGraphTest, DynamicCommandGroupHash) {
+  sycl::queue Queue;
+  experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
+
+  auto CGF = [&](handler &CGH) { CGH.parallel_for(1, [=](item<1> Item) {}); };
+
+  auto factory = [&]() {
+    return experimental::dynamic_command_group(Graph, {CGF});
+  };
+  ASSERT_NO_FATAL_FAILURE(
+      testHash<experimental::dynamic_command_group>(factory));
+}
+
+TEST_F(CommandGraphTest, DynamicParameterHash) {
+  sycl::queue Queue;
+  experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
+
+  auto factory = [&]() {
+    return experimental::dynamic_parameter<int>(Graph, 1);
+  };
+  ASSERT_NO_FATAL_FAILURE(
+      testHash<experimental::dynamic_parameter<int>>(factory));
+}

From eb2f79b519ad1e359a600ac9fd676d604d00bf80 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A1bio=20Mestre?= <fabio.mestre@codeplay.com>
Date: Mon, 27 Jan 2025 17:40:19 +0000
Subject: [PATCH 2/5] Update hash unit test

---
 .../CommandGraph/CommonReferenceSemantics.cpp | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/sycl/unittests/Extensions/CommandGraph/CommonReferenceSemantics.cpp b/sycl/unittests/Extensions/CommandGraph/CommonReferenceSemantics.cpp
index ee23782c03093..482bf99bc4fa0 100644
--- a/sycl/unittests/Extensions/CommandGraph/CommonReferenceSemantics.cpp
+++ b/sycl/unittests/Extensions/CommandGraph/CommonReferenceSemantics.cpp
@@ -110,26 +110,27 @@ void testHash(LambdaType &&objFactory) {
 
   const int NumObjects = 100;
 
-  std::unordered_map<T, bool> MapObjToBool{};
+  std::unordered_set<size_t> HashSet{};
+
   T Obj1 = objFactory();
   T Obj2 = objFactory();
   T Obj3 = objFactory();
   T Obj4 = objFactory();
 
-  ASSERT_TRUE(MapObjToBool.insert({Obj1, true}).second);
-  ASSERT_TRUE(MapObjToBool.insert({Obj2, true}).second);
+  ASSERT_TRUE(HashSet.insert(std::hash<T>{}(Obj1)).second);
+  ASSERT_TRUE(HashSet.insert(std::hash<T>{}(Obj2)).second);
 
-  // Insert objects and destroy them immediately to confirm that this doesn't
-  // create collisions with later insertions.
+  // Create objects and destroy them immediately to confirm that the
+  // hashes are unique and are not reused.
   for (int i = 0; i < NumObjects; ++i) {
     T ObjI = objFactory();
-    ASSERT_TRUE(MapObjToBool.insert({ObjI, true}).second);
+    ASSERT_TRUE(HashSet.insert(std::hash<T>{}(ObjI)).second);
   }
 
-  ASSERT_TRUE(MapObjToBool.insert({Obj3, true}).second);
-  ASSERT_TRUE(MapObjToBool.insert({Obj4, true}).second);
+  ASSERT_TRUE(HashSet.insert(std::hash<T>{}(Obj3)).second);
+  ASSERT_TRUE(HashSet.insert(std::hash<T>{}(Obj4)).second);
 
-  ASSERT_TRUE(MapObjToBool.size() == (NumObjects + 4));
+  ASSERT_TRUE(HashSet.size() == (NumObjects + 4));
 }
 
 TEST_F(CommandGraphTest, ModifiableGraphHash) {

From 8c9c0ea0c9664ca15aa64c5ae67e512077bbacb0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A1bio=20Mestre?= <fabio.mestre@codeplay.com>
Date: Wed, 29 Jan 2025 14:39:16 +0000
Subject: [PATCH 3/5] Address review comments

---
 sycl/include/sycl/ext/oneapi/experimental/graph.hpp |  2 +-
 sycl/source/detail/graph_impl.cpp                   |  4 ++--
 sycl/source/detail/graph_impl.hpp                   | 12 ++++++++----
 sycl/test/abi/sycl_symbols_windows.dump             |  6 ++++++
 4 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/sycl/include/sycl/ext/oneapi/experimental/graph.hpp b/sycl/include/sycl/ext/oneapi/experimental/graph.hpp
index ac0e713dfde95..8a530a3593770 100644
--- a/sycl/include/sycl/ext/oneapi/experimental/graph.hpp
+++ b/sycl/include/sycl/ext/oneapi/experimental/graph.hpp
@@ -17,7 +17,7 @@
 #ifdef __INTEL_PREVIEW_BREAKING_CHANGES
 #include <sycl/detail/string_view.hpp>
 #endif
-#include <sycl/device.hpp> // for device
+#include <sycl/device.hpp>                     // for device
 #include <sycl/ext/oneapi/experimental/detail/properties/graph_properties.hpp> // for graph properties classes
 #include <sycl/nd_range.hpp>                   // for range, nd_range
 #include <sycl/properties/property_traits.hpp> // for is_property, is_property_of
diff --git a/sycl/source/detail/graph_impl.cpp b/sycl/source/detail/graph_impl.cpp
index a498cc455bd85..9d9086a9a017e 100644
--- a/sycl/source/detail/graph_impl.cpp
+++ b/sycl/source/detail/graph_impl.cpp
@@ -2171,8 +2171,8 @@ size_t std::hash<sycl::ext::oneapi::experimental::node>::operator()(
 
 size_t
 std::hash<sycl::ext::oneapi::experimental::dynamic_command_group>::operator()(
-    const sycl::ext::oneapi::experimental::dynamic_command_group &DynamicCGH)
+    const sycl::ext::oneapi::experimental::dynamic_command_group &DynamicCG)
     const {
-  auto ID = sycl::detail::getSyclObjImpl(DynamicCGH)->getID();
+  auto ID = sycl::detail::getSyclObjImpl(DynamicCG)->getID();
   return std::hash<decltype(ID)>()(ID);
 }
diff --git a/sycl/source/detail/graph_impl.hpp b/sycl/source/detail/graph_impl.hpp
index 114b50a81d38e..1a1f2ef9cf55f 100644
--- a/sycl/source/detail/graph_impl.hpp
+++ b/sycl/source/detail/graph_impl.hpp
@@ -1120,7 +1120,7 @@ class graph_impl : public std::enable_shared_from_this<graph_impl> {
     return MBarrierDependencyMap[Queue];
   }
 
-  unsigned long long getID() { return MID; }
+  unsigned long long getID() const { return MID; }
 
 private:
   /// Iterate over the graph depth-first and run \p NodeFunc on each node.
@@ -1202,6 +1202,7 @@ class graph_impl : public std::enable_shared_from_this<graph_impl> {
       MBarrierDependencyMap;
 
   unsigned long long MID;
+  // Used for std::hash in order to create a unique hash for the instance.
   inline static std::atomic<unsigned long long> NextAvailableID = 0;
 };
 
@@ -1302,7 +1303,7 @@ class exec_graph_impl {
 
   void updateImpl(std::shared_ptr<node_impl> NodeImpl);
 
-  unsigned long long getID() { return MID; }
+  unsigned long long getID() const { return MID; }
 
 private:
   /// Create a command-group for the node and add it to command-buffer by going
@@ -1417,6 +1418,7 @@ class exec_graph_impl {
   std::multimap<node_impl::id_type, std::shared_ptr<node_impl>> MIDCache;
 
   unsigned long long MID;
+  // Used for std::hash in order to create a unique hash for the instance.
   inline static std::atomic<unsigned long long> NextAvailableID = 0;
 };
 
@@ -1505,7 +1507,7 @@ class dynamic_parameter_impl {
                                int ArgIndex,
                                const sycl::detail::AccessorBaseHost *Acc);
 
-  unsigned long long getID() { return MID; }
+  unsigned long long getID() const { return MID; }
 
   // Weak ptrs to node_impls which will be updated
   std::vector<std::pair<std::weak_ptr<node_impl>, int>> MNodes;
@@ -1517,6 +1519,7 @@ class dynamic_parameter_impl {
 
 private:
   unsigned long long MID;
+  // Used for std::hash in order to create a unique hash for the instance.
   inline static std::atomic<unsigned long long> NextAvailableID = 0;
 };
 
@@ -1559,10 +1562,11 @@ class dynamic_command_group_impl
   /// List of nodes using this dynamic command-group.
   std::vector<std::weak_ptr<node_impl>> MNodes;
 
-  unsigned long long getID() { return MID; }
+  unsigned long long getID() const { return MID; }
 
 private:
   unsigned long long MID;
+  // Used for std::hash in order to create a unique hash for the instance.
   inline static std::atomic<unsigned long long> NextAvailableID = 0;
 };
 } // namespace detail
diff --git a/sycl/test/abi/sycl_symbols_windows.dump b/sycl/test/abi/sycl_symbols_windows.dump
index a0649afb91058..9873df6476a87 100644
--- a/sycl/test/abi/sycl_symbols_windows.dump
+++ b/sycl/test/abi/sycl_symbols_windows.dump
@@ -334,6 +334,12 @@
 ??0dynamic_parameter_base@detail@experimental@oneapi@ext@_V1@sycl@@QEAA@$$QEAV0123456@@Z
 ??0dynamic_parameter_base@detail@experimental@oneapi@ext@_V1@sycl@@QEAA@AEBV0123456@@Z
 ??0dynamic_parameter_base@detail@experimental@oneapi@ext@_V1@sycl@@QEAA@V?$command_graph@$0A@@23456@_KPEBX@Z
+??4?$hash@Vdynamic_command_group@experimental@oneapi@ext@_V1@sycl@@@std@@QEAAAEAU01@AEBU01@@Z
+??4?$hash@Vdynamic_command_group@experimental@oneapi@ext@_V1@sycl@@@std@@QEAAAEAU01@$$QEAU01@@Z
+??R?$hash@Vdynamic_command_group@experimental@oneapi@ext@_V1@sycl@@@std@@QEBA_KAEBVdynamic_command_group@experimental@oneapi@ext@_V1@sycl@@@Z
+??R?$hash@Vnode@experimental@oneapi@ext@_V1@sycl@@@std@@QEBA_KAEBVnode@experimental@oneapi@ext@_V1@sycl@@@Z
+??4?$hash@Vnode@experimental@oneapi@ext@_V1@sycl@@@std@@QEAAAEAU01@$$QEAU01@@Z
+??4?$hash@Vnode@experimental@oneapi@ext@_V1@sycl@@@std@@QEAAAEAU01@AEBU01@@Z
 ??0event@_V1@sycl@@AEAA@V?$shared_ptr@Vevent_impl@detail@_V1@sycl@@@std@@@Z
 ??0event@_V1@sycl@@QEAA@$$QEAV012@@Z
 ??0event@_V1@sycl@@QEAA@AEBV012@@Z

From a5bfd9a10367d79af7efd6a95d7e5f4f2056410e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A1bio=20Mestre?= <fabio.mestre@codeplay.com>
Date: Wed, 29 Jan 2025 15:27:23 +0000
Subject: [PATCH 4/5] Addres review comments

---
 .../CommandGraph/CommonReferenceSemantics.cpp | 64 +++++++++----------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/sycl/unittests/Extensions/CommandGraph/CommonReferenceSemantics.cpp b/sycl/unittests/Extensions/CommandGraph/CommonReferenceSemantics.cpp
index 482bf99bc4fa0..9451bf1334b12 100644
--- a/sycl/unittests/Extensions/CommandGraph/CommonReferenceSemantics.cpp
+++ b/sycl/unittests/Extensions/CommandGraph/CommonReferenceSemantics.cpp
@@ -1,4 +1,4 @@
-//==----------------------- CommandGraph.cpp -------------------------------==//
+//==--------------------- CommonReferenceSemantics.cpp ---------------------==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -15,13 +15,13 @@ using namespace sycl::ext::oneapi;
 /**
  * Checks that the operators and constructors of graph related classes meet the
  * common reference semantics.
- * @param objFactory A function object that returns an object to be tested.
+ * @param ObjFactory A function object that returns an object to be tested.
  */
 template <typename T, typename LambdaType>
-void testSemantics(LambdaType &&objFactory) {
+void testSemantics(LambdaType &&ObjFactory) {
 
-  T Obj1 = objFactory();
-  T Obj2 = objFactory();
+  T Obj1 = ObjFactory();
+  T Obj2 = ObjFactory();
 
   // Check the == and != operators.
   ASSERT_FALSE(Obj1 == Obj2);
@@ -42,38 +42,38 @@ void testSemantics(LambdaType &&objFactory) {
 
 TEST_F(CommandGraphTest, ModifiableGraphSemantics) {
   sycl::queue Queue;
-  auto factory = [&]() {
+  auto Factory = [&]() {
     return experimental::command_graph(Queue.get_context(), Queue.get_device());
   };
 
   ASSERT_NO_FATAL_FAILURE(
       testSemantics<
           experimental::command_graph<experimental::graph_state::modifiable>>(
-          factory));
+          Factory));
 }
 
 TEST_F(CommandGraphTest, ExecutableGraphSemantics) {
   sycl::queue Queue;
 
-  auto factory = [&]() {
+  auto Factory = [&]() {
     experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
     return Graph.finalize();
   };
   ASSERT_NO_FATAL_FAILURE(
       testSemantics<
           experimental::command_graph<experimental::graph_state::executable>>(
-          factory));
+          Factory));
 }
 
 TEST_F(CommandGraphTest, NodeSemantics) {
   sycl::queue Queue;
   experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
 
-  auto factory = [&]() {
+  auto Factory = [&]() {
     return Graph.add(
         [&](handler &CGH) { CGH.parallel_for(1, [=](item<1> Item) {}); });
   };
-  ASSERT_NO_FATAL_FAILURE(testSemantics<experimental::node>(factory));
+  ASSERT_NO_FATAL_FAILURE(testSemantics<experimental::node>(Factory));
 }
 
 TEST_F(CommandGraphTest, DynamicCGSemantics) {
@@ -82,40 +82,40 @@ TEST_F(CommandGraphTest, DynamicCGSemantics) {
 
   auto CGF = [&](handler &CGH) { CGH.parallel_for(1, [=](item<1> Item) {}); };
 
-  auto factory = [&]() {
+  auto Factory = [&]() {
     return experimental::dynamic_command_group(Graph, {CGF});
   };
   ASSERT_NO_FATAL_FAILURE(
-      testSemantics<experimental::dynamic_command_group>(factory));
+      testSemantics<experimental::dynamic_command_group>(Factory));
 }
 
 TEST_F(CommandGraphTest, DynamicParamSemantics) {
   sycl::queue Queue;
   experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
 
-  auto factory = [&]() {
+  auto Factory = [&]() {
     return experimental::dynamic_parameter<int>(Graph, 1);
   };
   ASSERT_NO_FATAL_FAILURE(
-      testSemantics<experimental::dynamic_parameter<int>>(factory));
+      testSemantics<experimental::dynamic_parameter<int>>(Factory));
 }
 
 /**
  * Checks for potential hash collisions in the hash implementations of graph
  * related classes.
- * @param objFactory A function object that returns an object to be tested.
+ * @param ObjFactory A function object that returns an object to be tested.
  */
 template <typename T, typename LambdaType>
-void testHash(LambdaType &&objFactory) {
+void testHash(LambdaType &&ObjFactory) {
 
   const int NumObjects = 100;
 
   std::unordered_set<size_t> HashSet{};
 
-  T Obj1 = objFactory();
-  T Obj2 = objFactory();
-  T Obj3 = objFactory();
-  T Obj4 = objFactory();
+  T Obj1 = ObjFactory();
+  T Obj2 = ObjFactory();
+  T Obj3 = ObjFactory();
+  T Obj4 = ObjFactory();
 
   ASSERT_TRUE(HashSet.insert(std::hash<T>{}(Obj1)).second);
   ASSERT_TRUE(HashSet.insert(std::hash<T>{}(Obj2)).second);
@@ -123,7 +123,7 @@ void testHash(LambdaType &&objFactory) {
   // Create objects and destroy them immediately to confirm that the
   // hashes are unique and are not reused.
   for (int i = 0; i < NumObjects; ++i) {
-    T ObjI = objFactory();
+    T ObjI = ObjFactory();
     ASSERT_TRUE(HashSet.insert(std::hash<T>{}(ObjI)).second);
   }
 
@@ -135,38 +135,38 @@ void testHash(LambdaType &&objFactory) {
 
 TEST_F(CommandGraphTest, ModifiableGraphHash) {
   sycl::queue Queue;
-  auto factory = [&]() {
+  auto Factory = [&]() {
     return experimental::command_graph(Queue.get_context(), Queue.get_device());
   };
 
   ASSERT_NO_FATAL_FAILURE(
       testHash<
           experimental::command_graph<experimental::graph_state::modifiable>>(
-          factory));
+          Factory));
 }
 
 TEST_F(CommandGraphTest, ExecutableGraphHash) {
   sycl::queue Queue;
 
-  auto factory = [&]() {
+  auto Factory = [&]() {
     experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
     return Graph.finalize();
   };
   ASSERT_NO_FATAL_FAILURE(
       testHash<
           experimental::command_graph<experimental::graph_state::executable>>(
-          factory));
+          Factory));
 }
 
 TEST_F(CommandGraphTest, NodeHash) {
   sycl::queue Queue;
   experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
 
-  auto factory = [&]() {
+  auto Factory = [&]() {
     return Graph.add(
         [&](handler &CGH) { CGH.parallel_for(1, [=](item<1> Item) {}); });
   };
-  ASSERT_NO_FATAL_FAILURE(testHash<experimental::node>(factory));
+  ASSERT_NO_FATAL_FAILURE(testHash<experimental::node>(Factory));
 }
 
 TEST_F(CommandGraphTest, DynamicCommandGroupHash) {
@@ -175,20 +175,20 @@ TEST_F(CommandGraphTest, DynamicCommandGroupHash) {
 
   auto CGF = [&](handler &CGH) { CGH.parallel_for(1, [=](item<1> Item) {}); };
 
-  auto factory = [&]() {
+  auto Factory = [&]() {
     return experimental::dynamic_command_group(Graph, {CGF});
   };
   ASSERT_NO_FATAL_FAILURE(
-      testHash<experimental::dynamic_command_group>(factory));
+      testHash<experimental::dynamic_command_group>(Factory));
 }
 
 TEST_F(CommandGraphTest, DynamicParameterHash) {
   sycl::queue Queue;
   experimental::command_graph Graph(Queue.get_context(), Queue.get_device());
 
-  auto factory = [&]() {
+  auto Factory = [&]() {
     return experimental::dynamic_parameter<int>(Graph, 1);
   };
   ASSERT_NO_FATAL_FAILURE(
-      testHash<experimental::dynamic_parameter<int>>(factory));
+      testHash<experimental::dynamic_parameter<int>>(Factory));
 }

From 20aff4acdd01b6ad1869c80e97f2d4ef46b13eb7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A1bio=20Mestre?= <fabio.mestre@codeplay.com>
Date: Fri, 31 Jan 2025 15:00:33 +0000
Subject: [PATCH 5/5] Address review comments

---
 sycl/include/sycl/ext/oneapi/experimental/graph.hpp | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sycl/include/sycl/ext/oneapi/experimental/graph.hpp b/sycl/include/sycl/ext/oneapi/experimental/graph.hpp
index 8a530a3593770..8cef9fa1be9d4 100644
--- a/sycl/include/sycl/ext/oneapi/experimental/graph.hpp
+++ b/sycl/include/sycl/ext/oneapi/experimental/graph.hpp
@@ -147,7 +147,7 @@ class __SYCL_EXPORT node {
     return LHS.impl == RHS.impl;
   }
   friend bool operator!=(const node &LHS, const node &RHS) {
-    return LHS.impl != RHS.impl;
+    return !operator==(LHS, RHS);
   }
 
 private:
@@ -196,7 +196,7 @@ class __SYCL_EXPORT dynamic_command_group {
   }
   friend bool operator!=(const dynamic_command_group &LHS,
                          const dynamic_command_group &RHS) {
-    return LHS.impl != RHS.impl;
+    return !operator==(LHS, RHS);
   }
 
 private:
@@ -332,7 +332,7 @@ class __SYCL_EXPORT modifiable_command_graph
   }
   friend bool operator!=(const modifiable_command_graph &LHS,
                          const modifiable_command_graph &RHS) {
-    return LHS.impl != RHS.impl;
+    return !operator==(LHS, RHS);
   }
 
 protected:
@@ -421,7 +421,7 @@ class __SYCL_EXPORT executable_command_graph
   }
   friend bool operator!=(const executable_command_graph &LHS,
                          const executable_command_graph &RHS) {
-    return LHS.impl != RHS.impl;
+    return !operator==(LHS, RHS);
   }
 
 protected:
@@ -497,7 +497,7 @@ class __SYCL_EXPORT dynamic_parameter_base {
   }
   friend bool operator!=(const dynamic_parameter_base &LHS,
                          const dynamic_parameter_base &RHS) {
-    return LHS.impl != RHS.impl;
+    return !operator==(LHS, RHS);
   }
 
 protected: