diff --git a/sycl/doc/design/CommandGraph.md b/sycl/doc/design/CommandGraph.md index 87c4a57bb5c4f..c1e570d13ab21 100644 --- a/sycl/doc/design/CommandGraph.md +++ b/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 diff --git a/sycl/source/detail/graph_impl.cpp b/sycl/source/detail/graph_impl.cpp index 237d63f7ec463..30f7b68e7639e 100644 --- a/sycl/source/detail/graph_impl.cpp +++ b/sycl/source/detail/graph_impl.cpp @@ -1171,23 +1171,25 @@ void exec_graph_impl::update(std::shared_ptr 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"); } } } diff --git a/sycl/test-e2e/Graph/Inputs/whole_update_double_buffer.cpp b/sycl/test-e2e/Graph/Inputs/whole_update_double_buffer.cpp index 10c2e45b6f5b5..d3aa5068104e2 100644 --- a/sycl/test-e2e/Graph/Inputs/whole_update_double_buffer.cpp +++ b/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 diff --git a/sycl/test-e2e/Graph/Explicit/whole_update_double_buffer.cpp b/sycl/test-e2e/Graph/Update/Explicit/whole_update_double_buffer.cpp similarity index 90% rename from sycl/test-e2e/Graph/Explicit/whole_update_double_buffer.cpp rename to sycl/test-e2e/Graph/Update/Explicit/whole_update_double_buffer.cpp index 5b5ab7626b44c..31324152efade 100644 --- a/sycl/test-e2e/Graph/Explicit/whole_update_double_buffer.cpp +++ b/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" diff --git a/sycl/test-e2e/Graph/Explicit/whole_update_subgraph.cpp b/sycl/test-e2e/Graph/Update/Explicit/whole_update_subgraph.cpp similarity index 92% rename from sycl/test-e2e/Graph/Explicit/whole_update_subgraph.cpp rename to sycl/test-e2e/Graph/Update/Explicit/whole_update_subgraph.cpp index 0ea0ccc34fc60..9c6abb96bec71 100644 --- a/sycl/test-e2e/Graph/Explicit/whole_update_subgraph.cpp +++ b/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" diff --git a/sycl/test-e2e/Graph/Explicit/whole_update_usm.cpp b/sycl/test-e2e/Graph/Update/Explicit/whole_update_usm.cpp similarity index 92% rename from sycl/test-e2e/Graph/Explicit/whole_update_usm.cpp rename to sycl/test-e2e/Graph/Update/Explicit/whole_update_usm.cpp index 50c6bdbe86df6..743c5ed4d2bc9 100644 --- a/sycl/test-e2e/Graph/Explicit/whole_update_usm.cpp +++ b/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" diff --git a/sycl/test-e2e/Graph/RecordReplay/whole_update_double_buffer.cpp b/sycl/test-e2e/Graph/Update/RecordReplay/whole_update_double_buffer.cpp similarity index 90% rename from sycl/test-e2e/Graph/RecordReplay/whole_update_double_buffer.cpp rename to sycl/test-e2e/Graph/Update/RecordReplay/whole_update_double_buffer.cpp index 84e6c96c979c1..126604f177f32 100644 --- a/sycl/test-e2e/Graph/RecordReplay/whole_update_double_buffer.cpp +++ b/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" diff --git a/sycl/test-e2e/Graph/RecordReplay/whole_update_subgraph.cpp b/sycl/test-e2e/Graph/Update/RecordReplay/whole_update_subgraph.cpp similarity index 92% rename from sycl/test-e2e/Graph/RecordReplay/whole_update_subgraph.cpp rename to sycl/test-e2e/Graph/Update/RecordReplay/whole_update_subgraph.cpp index dce4628d73bb3..73c49b0971d83 100644 --- a/sycl/test-e2e/Graph/RecordReplay/whole_update_subgraph.cpp +++ b/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" diff --git a/sycl/test-e2e/Graph/RecordReplay/whole_update_usm.cpp b/sycl/test-e2e/Graph/Update/RecordReplay/whole_update_usm.cpp similarity index 92% rename from sycl/test-e2e/Graph/RecordReplay/whole_update_usm.cpp rename to sycl/test-e2e/Graph/Update/RecordReplay/whole_update_usm.cpp index cd5afb58bfd7f..94ca2c08d94ef 100644 --- a/sycl/test-e2e/Graph/RecordReplay/whole_update_usm.cpp +++ b/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" diff --git a/sycl/unittests/Extensions/CommandGraph/Update.cpp b/sycl/unittests/Extensions/CommandGraph/Update.cpp index 44814b6a54f60..60bb64df3be00 100644 --- a/sycl/unittests/Extensions/CommandGraph/Update.cpp +++ b/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) {