Address review comments

Author: fabiomestre

sycl/doc/design/CommandGraph.md

@@ -234,9 +234,7 @@ yet been implemented.
 
 ### Design Challenges
 
-#### Explicit Update
-
-Explicit updates of individual nodes faces significant design challenges in SYCL:
+Graph update faces significant design challenges in SYCL:
 
 * Lambda capture order is explicitly undefined in C++, so the user cannot reason
   about the indices of arguments captured by kernel lambdas.
@@ -258,18 +256,9 @@ can be used:
   extension](../extensions/proposed/sycl_ext_oneapi_free_function_kernels.asciidoc)
 * OpenCL interop kernels created from SPIR-V source at runtime.
 
-A workaround for the lambda capture issues is the "Whole-Graph Update" feature.
-Since the lambda capture order is the same across two different recordings, we
-can match the parameter order when updating.
-
-#### Whole-Graph Update
-
-The current implementation of the whole-graph update feature relies on the
-assumption that both graphs should have a similar topology. Currently, the
-implementation only checks that both graphs have an identical number of nodes
-and that each node contains the same number of edges. A possible design change
-could be to add more checks to the implementation. This would give the user
-better error messages but with possible performance penalties.
+A possible future workaround lambda capture issues could be "Whole-Graph Update"
+where if we can guarantee that lambda capture order is the same across two
+different recordings we can then match parameter order when updating.
 
 ### Scheduler Integration
 

sycl/source/detail/graph_impl.cpp

@@ -1171,23 +1171,25 @@ void exec_graph_impl::update(std::shared_ptr<graph_impl> GraphImpl) {
 
   if (MNodeStorage.size() != GraphImpl->MNodeStorage.size()) {
     throw sycl::exception(sycl::make_error_code(errc::invalid),
-                          "Mismatch found in the number of nodes. The graphs "
-                          "must have a matching topology.");
+                          "Cannot update using a graph with a different "
+                          "topology. Mismatch found in the number of nodes.");
   } else {
     for (uint32_t i = 0; i < MNodeStorage.size(); ++i) {
       if (MNodeStorage[i]->MSuccessors.size() !=
               GraphImpl->MNodeStorage[i]->MSuccessors.size() ||
           MNodeStorage[i]->MPredecessors.size() !=
               GraphImpl->MNodeStorage[i]->MPredecessors.size()) {
-        throw sycl::exception(sycl::make_error_code(errc::invalid),
-                              "Mismatch found in the number of edges. The "
-                              "graphs must have a matching topology.");
+        throw sycl::exception(
+            sycl::make_error_code(errc::invalid),
+            "Cannot update using a graph with a different topology. Mismatch "
+            "found in the number of edges.");
       }
 
       if (MNodeStorage[i]->MCGType != GraphImpl->MNodeStorage[i]->MCGType) {
-        throw sycl::exception(sycl::make_error_code(errc::invalid),
-                              "Mismatch found in the type of nodes. Each pair "
-                              "of nodes being updated must have the same type");
+        throw sycl::exception(
+            sycl::make_error_code(errc::invalid),
+            "Cannot update using a graph with mismatched node types. Each pair "
+            "of nodes being updated must have the same type");
       }
     }
   }

sycl/test-e2e/Graph/Inputs/whole_update_double_buffer.cpp

@@ -61,13 +61,11 @@ int main() {
     event Event;
     for (size_t i = 0; i < Iterations; i++) {
       Event = Queue.submit([&](handler &CGH) {
-        CGH.depends_on(Event);
         CGH.ext_oneapi_graph(ExecGraph);
       });
       // Update to second set of buffers
       ExecGraph.update(GraphUpdate);
       Event = Queue.submit([&](handler &CGH) {
-        CGH.depends_on(Event);
         CGH.ext_oneapi_graph(ExecGraph);
       });
       // Reset back to original buffers

sycl/test-e2e/Graph/Explicit/whole_update_double_buffer.cpp sycl/test-e2e/Graph/Update/Explicit/whole_update_double_buffer.cpp

@@ -7,4 +7,4 @@
 
 #define GRAPH_E2E_EXPLICIT
 
-#include "../Inputs/whole_update_double_buffer.cpp"
+#include "../../Inputs/whole_update_double_buffer.cpp"

sycl/test-e2e/Graph/Explicit/whole_update_subgraph.cpp sycl/test-e2e/Graph/Update/Explicit/whole_update_subgraph.cpp

@@ -9,4 +9,4 @@
 
 #define GRAPH_E2E_EXPLICIT
 
-#include "../Inputs/whole_update_subgraph.cpp"
+#include "../../Inputs/whole_update_subgraph.cpp"

sycl/test-e2e/Graph/Explicit/whole_update_usm.cpp sycl/test-e2e/Graph/Update/Explicit/whole_update_usm.cpp

@@ -7,4 +7,4 @@
 
 #define GRAPH_E2E_EXPLICIT
 
-#include "../Inputs/whole_update_usm.cpp"
+#include "../../Inputs/whole_update_usm.cpp"

sycl/test-e2e/Graph/RecordReplay/whole_update_double_buffer.cpp sycl/test-e2e/Graph/Update/RecordReplay/whole_update_double_buffer.cpp

@@ -7,4 +7,4 @@
 
 #define GRAPH_E2E_RECORD_REPLAY
 
-#include "../Inputs/whole_update_double_buffer.cpp"
+#include "../../Inputs/whole_update_double_buffer.cpp"

sycl/test-e2e/Graph/RecordReplay/whole_update_subgraph.cpp sycl/test-e2e/Graph/Update/RecordReplay/whole_update_subgraph.cpp

@@ -9,4 +9,4 @@
 
 #define GRAPH_E2E_RECORD_REPLAY
 
-#include "../Inputs/whole_update_subgraph.cpp"
+#include "../../Inputs/whole_update_subgraph.cpp"

sycl/test-e2e/Graph/RecordReplay/whole_update_usm.cpp sycl/test-e2e/Graph/Update/RecordReplay/whole_update_usm.cpp

@@ -9,4 +9,4 @@
 
 #define GRAPH_E2E_RECORD_REPLAY
 
-#include "../Inputs/whole_update_usm.cpp"
+#include "../../Inputs/whole_update_usm.cpp"

sycl/unittests/Extensions/CommandGraph/Update.cpp

@@ -290,9 +290,34 @@ TEST_F(WholeGraphUpdateTest, MissingEdges) {
   EXPECT_THROW(GraphExec.update(UpdateGraph), sycl::exception);
 }
 
-TEST_F(WholeGraphUpdateTest, WrongOrderEdges) {
+TEST_F(WholeGraphUpdateTest, DISABLED_WrongOrderNodes) {
+  // Test that using an update graph with nodes added in a different order
+  // results in an error.
+
+  auto NodeA = Graph.add(EmptyKernel);
+  auto NodeB =
+      Graph.add(EmptyKernel, experimental::property::node::depends_on(NodeA));
+  auto NodeC =
+      Graph.add(EmptyKernel, experimental::property::node::depends_on(NodeA));
+  auto NodeD = Graph.add(
+      EmptyKernel, experimental::property::node::depends_on(NodeB, NodeC));
+
+  auto UpdateNodeA = UpdateGraph.add(EmptyKernel);
+  auto UpdateNodeC = UpdateGraph.add(
+      EmptyKernel, experimental::property::node::depends_on(UpdateNodeA));
+  auto UpdateNodeB = UpdateGraph.add(
+      EmptyKernel, experimental::property::node::depends_on(UpdateNodeA));
+  auto UpdateNodeD = UpdateGraph.add(
+      EmptyKernel,
+      experimental::property::node::depends_on(UpdateNodeB, UpdateNodeC));
+
+  auto GraphExec = Graph.finalize(experimental::property::graph::updatable{});
+  EXPECT_THROW(GraphExec.update(UpdateGraph), sycl::exception);
+}
+
+TEST_F(WholeGraphUpdateTest, DISABLED_WrongOrderEdges) {
   // Test that using an update graph with edges added in a different order
-  // does not result in an error.
+  // results in an error.
 
   auto NodeA = Graph.add(EmptyKernel);
   auto NodeB = Graph.add(EmptyKernel);
@@ -316,7 +341,7 @@ TEST_F(WholeGraphUpdateTest, WrongOrderEdges) {
   UpdateGraph.make_edge(UpdateNodeB, UpdateNodeD);
 
   auto GraphExec = Graph.finalize(experimental::property::graph::updatable{});
-  EXPECT_NO_THROW(GraphExec.update(UpdateGraph));
+  EXPECT_THROW(GraphExec.update(UpdateGraph), sycl::exception);
 }
 
 TEST_F(WholeGraphUpdateTest, UnsupportedNodeType) {