From 1ad34f37476c4eb12033e9ae44e26ad3bcac86ac Mon Sep 17 00:00:00 2001 From: Jorge Pineda Date: Wed, 3 Apr 2024 12:13:24 -0700 Subject: [PATCH 1/2] [ET-VK] Introduce add_tensor overloads consuming TensorRef From @ssjia: > we should always make sure to store references produced from `graph.get_val()` only after any calls to `graph.add_*()` (i.e. modifications to the values list) are made. This is because `graph.values_`, being a `std::vector`, will reallocate with more space and move its contents if the current allocation is not sufficient. This means that if you store a reference then call `graph.add_*()` then the underlying resource the reference points to may have been moved. I think we can guard against this behavior by passing a `TensorRef` directly, and never having to declare a variable `TensorRef& tref` in the caller's scope. An example is shown in `Staging.cpp`. We could have it consume `ValueRef` for brevity of the passing parameter but IMO it hinders readability. Differential Revision: [D55703483](https://our.internmc.facebook.com/intern/diff/D55703483/) [ghstack-poisoned] --- .../vulkan/runtime/graph/ComputeGraph.cpp | 19 +++++++++++++----- backends/vulkan/runtime/graph/ComputeGraph.h | 20 +++++++++++++++++-- .../vulkan/runtime/graph/ops/impl/Staging.cpp | 3 +-- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/backends/vulkan/runtime/graph/ComputeGraph.cpp b/backends/vulkan/runtime/graph/ComputeGraph.cpp index dada914b22d..da884c86085 100644 --- a/backends/vulkan/runtime/graph/ComputeGraph.cpp +++ b/backends/vulkan/runtime/graph/ComputeGraph.cpp @@ -123,6 +123,13 @@ ValueRef ComputeGraph::add_tensor( return idx; } +ValueRef ComputeGraph::add_tensor( + TensorRef& tref, + const api::StorageType storage_type, + const api::GPUMemoryLayout memory_layout) { + return add_tensor(tref.sizes, tref.dtype, storage_type, memory_layout); +} + ValueRef ComputeGraph::add_tensor( const std::vector& sizes, const api::ScalarType dtype, @@ -132,16 +139,18 @@ ValueRef ComputeGraph::add_tensor( sizes, dtype, suggested_storage_type(), memory_layout, shared_object_idx); } +ValueRef ComputeGraph::add_tensor( + TensorRef& tref, + const api::GPUMemoryLayout memory_layout) { + return add_tensor(tref.sizes, tref.dtype, memory_layout); +} + ValueRef ComputeGraph::add_tensor( const std::vector& sizes, const api::ScalarType dtype, const int64_t shared_object_idx) { return add_tensor( - sizes, - dtype, - suggested_storage_type(), - suggested_memory_layout(sizes), - shared_object_idx); + sizes, dtype, suggested_memory_layout(sizes), shared_object_idx); } ValueRef ComputeGraph::add_tensorref( diff --git a/backends/vulkan/runtime/graph/ComputeGraph.h b/backends/vulkan/runtime/graph/ComputeGraph.h index 00aa60020f3..7a9439be128 100644 --- a/backends/vulkan/runtime/graph/ComputeGraph.h +++ b/backends/vulkan/runtime/graph/ComputeGraph.h @@ -172,7 +172,15 @@ class ComputeGraph final { const api::ScalarType dtype, const api::StorageType storage_type, const api::GPUMemoryLayout memory_layout, - const int64_t shared_object_idx); + const int64_t shared_object_idx = -1); + + /* + * Add a `vTensor` value to the graph with the properties of `tref`. + */ + ValueRef add_tensor( + TensorRef& tref, + const api::StorageType storage_type, + const api::GPUMemoryLayout memory_layout); /* * Add a `vTensor` value to the graph with the specified properties. The @@ -184,6 +192,14 @@ class ComputeGraph final { const api::GPUMemoryLayout memory_layout, const int64_t shared_object_idx = -1); + /* + * Add a `vTensor` value to the graph with the properties of `tref`. The + * suggested storage type will be used to construct the `vTensor`. + */ + ValueRef add_tensor( + TensorRef& tref, + const api::GPUMemoryLayout memory_layout); + /* * Add a `vTensor` value to the graph with the specified properties. The * suggested storage type and memory layout will be used to construct the @@ -191,7 +207,7 @@ class ComputeGraph final { */ ValueRef add_tensor( const std::vector& sizes, - const api::ScalarType dtype = api::ScalarType::Float, + const api::ScalarType dtype, const int64_t shared_object_idx = -1); /* diff --git a/backends/vulkan/runtime/graph/ops/impl/Staging.cpp b/backends/vulkan/runtime/graph/ops/impl/Staging.cpp index e08fad5df83..22ba40aad3b 100644 --- a/backends/vulkan/runtime/graph/ops/impl/Staging.cpp +++ b/backends/vulkan/runtime/graph/ops/impl/Staging.cpp @@ -63,8 +63,7 @@ ValueRef prepack( ComputeGraph& graph, const ValueRef vref, const api::GPUMemoryLayout layout) { - TensorRef& tref = graph.get_val(vref).toTensorRef(); - ValueRef v = graph.add_tensor(tref.sizes, tref.dtype, layout); + ValueRef v = graph.add_tensor(graph.get_val(vref).toTensorRef(), layout); vTensor& t = graph.get_val(v).toTensor(); api::ShaderInfo shader = get_nchw_to_image_shader(t); From 49d72c8922556612d2d44de7afac63c3351b98ff Mon Sep 17 00:00:00 2001 From: Jorge Pineda Date: Wed, 3 Apr 2024 19:35:44 -0700 Subject: [PATCH 2/2] Update on "[ET-VK] Introduce add_tensor overloads consuming TensorRef" From ssjia: > we should always make sure to store references produced from `graph.get_val()` only after any calls to `graph.add_*()` (i.e. modifications to the values list) are made. This is because `graph.values_`, being a `std::vector`, will reallocate with more space and move its contents if the current allocation is not sufficient. This means that if you store a reference then call `graph.add_*()` then the underlying resource the reference points to may have been moved. I think we can guard against this behavior by passing a `TensorRef` directly, and never having to declare a variable `TensorRef& tref` in the caller's scope. An example is shown in `Staging.cpp`. We could have it consume `ValueRef` for brevity of the passing parameter but IMO it hinders readability. Differential Revision: [D55703483](https://our.internmc.facebook.com/intern/diff/D55703483/) [ghstack-poisoned] --- .../vulkan/runtime/graph/ComputeGraph.cpp | 20 ++++++------ backends/vulkan/runtime/graph/ComputeGraph.h | 32 +++++++++---------- .../vulkan/runtime/graph/ops/impl/Staging.cpp | 2 +- 3 files changed, 28 insertions(+), 26 deletions(-) diff --git a/backends/vulkan/runtime/graph/ComputeGraph.cpp b/backends/vulkan/runtime/graph/ComputeGraph.cpp index da884c86085..789a0945366 100644 --- a/backends/vulkan/runtime/graph/ComputeGraph.cpp +++ b/backends/vulkan/runtime/graph/ComputeGraph.cpp @@ -123,13 +123,6 @@ ValueRef ComputeGraph::add_tensor( return idx; } -ValueRef ComputeGraph::add_tensor( - TensorRef& tref, - const api::StorageType storage_type, - const api::GPUMemoryLayout memory_layout) { - return add_tensor(tref.sizes, tref.dtype, storage_type, memory_layout); -} - ValueRef ComputeGraph::add_tensor( const std::vector& sizes, const api::ScalarType dtype, @@ -139,9 +132,18 @@ ValueRef ComputeGraph::add_tensor( sizes, dtype, suggested_storage_type(), memory_layout, shared_object_idx); } -ValueRef ComputeGraph::add_tensor( - TensorRef& tref, +ValueRef ComputeGraph::add_tensor_like( + const ValueRef vref, + const api::StorageType storage_type, + const api::GPUMemoryLayout memory_layout) { + TensorRef& tref = get_val(vref).toTensorRef(); + return add_tensor(tref.sizes, tref.dtype, storage_type, memory_layout); +} + +ValueRef ComputeGraph::add_tensor_like( + const ValueRef vref, const api::GPUMemoryLayout memory_layout) { + TensorRef& tref = get_val(vref).toTensorRef(); return add_tensor(tref.sizes, tref.dtype, memory_layout); } diff --git a/backends/vulkan/runtime/graph/ComputeGraph.h b/backends/vulkan/runtime/graph/ComputeGraph.h index 7a9439be128..24117d39f9f 100644 --- a/backends/vulkan/runtime/graph/ComputeGraph.h +++ b/backends/vulkan/runtime/graph/ComputeGraph.h @@ -174,14 +174,6 @@ class ComputeGraph final { const api::GPUMemoryLayout memory_layout, const int64_t shared_object_idx = -1); - /* - * Add a `vTensor` value to the graph with the properties of `tref`. - */ - ValueRef add_tensor( - TensorRef& tref, - const api::StorageType storage_type, - const api::GPUMemoryLayout memory_layout); - /* * Add a `vTensor` value to the graph with the specified properties. The * suggested storage type will be used to construct the `vTensor`. @@ -192,14 +184,6 @@ class ComputeGraph final { const api::GPUMemoryLayout memory_layout, const int64_t shared_object_idx = -1); - /* - * Add a `vTensor` value to the graph with the properties of `tref`. The - * suggested storage type will be used to construct the `vTensor`. - */ - ValueRef add_tensor( - TensorRef& tref, - const api::GPUMemoryLayout memory_layout); - /* * Add a `vTensor` value to the graph with the specified properties. The * suggested storage type and memory layout will be used to construct the @@ -210,6 +194,22 @@ class ComputeGraph final { const api::ScalarType dtype, const int64_t shared_object_idx = -1); + /* + * Add a `vTensor` value to the graph with the properties of `vref`. + */ + ValueRef add_tensor_like( + const ValueRef vref, + const api::StorageType storage_type, + const api::GPUMemoryLayout memory_layout); + + /* + * Add a `vTensor` value to the graph with the properties of `vref`. The + * suggested storage type will be used to construct the `vTensor`. + */ + ValueRef add_tensor_like( + const ValueRef vref, + const api::GPUMemoryLayout memory_layout); + /* * Add a `TensorRef` value to the graph with the specific properties. A * `TensorRef` is a reference to a `vTensor` whose data is stored in an diff --git a/backends/vulkan/runtime/graph/ops/impl/Staging.cpp b/backends/vulkan/runtime/graph/ops/impl/Staging.cpp index 22ba40aad3b..7d646a27111 100644 --- a/backends/vulkan/runtime/graph/ops/impl/Staging.cpp +++ b/backends/vulkan/runtime/graph/ops/impl/Staging.cpp @@ -63,7 +63,7 @@ ValueRef prepack( ComputeGraph& graph, const ValueRef vref, const api::GPUMemoryLayout layout) { - ValueRef v = graph.add_tensor(graph.get_val(vref).toTensorRef(), layout); + ValueRef v = graph.add_tensor_like(vref, layout); vTensor& t = graph.get_val(v).toTensor(); api::ShaderInfo shader = get_nchw_to_image_shader(t);