From eda6dc39f2b79cc81d58256b7b37fc38447f4f50 Mon Sep 17 00:00:00 2001 From: Kenneth Moreland Date: Fri, 13 Jan 2023 15:23:30 -0700 Subject: [PATCH] Support using arrays with dynamic Vec-likes as output arrays When you use an `ArrayHandle` as an output array in a worklet (for example, as a `FieldOut`), the fetch operation does not read values from the array during the `Load`. Instead, it just constructs a new object. This makes sense as an output array is expected to have garbage in it anyway. This is a problem for some special arrays that contain `Vec`-like objects that are sized dynamically. For example, if you use an `ArrayHandleGroupVecVariable`, each entry is a dynamically sized `Vec`. The array is referenced by creating a special version of `Vec` that holds a reference to the array portal and an index. Components are retrieved and set by accessing the memory in the array portal. This allows us to have a dynamically sized `Vec` in the execution environment without having to allocate within the worklet. The problem comes when we want to use one of these arrays with `Vec`-like objects for an output. The typical fetch fails because you cannot construct one of these `Vec`-like objects without an array portal to bind it to. In these cases, we need the fetch to create the `Vec`-like object by reading it from the array. Even though the data will be garbage, you get the necessary buffer into the array (and nothing more). Previously, the problem was fixed by creating partial specializations of the `Fetch` for these `ArrayHandle`s. This worked OK as long as you were using the array directly. However, the approach failed if the `ArrayHandle` was wrapped in another `ArrayHandle` (for example, if an `ArrayHandleView` was applied to an `ArrayHandleGroupVecVariable`). To get around this problem and simplify things, the basic `Fetch` for direct output arrays is changed to handle all cases where the values in the `ArrayHandle` cannot be directly constructed. A compile-time check of the array's value type is checked with `std::is_default_constructible`. If it can be constructed, then the array is not accessed. If it cannot be constructed, then it grabs a value out of the array. --- docs/changelog/output-vec-like-fix.md | 36 ++++++++++ vtkm/VecFromPortal.h | 8 --- vtkm/cont/ArrayHandleGroupVecVariable.h | 4 -- vtkm/cont/ArrayHandleRecombineVec.h | 13 ++-- .../UnitTestArrayHandleRecombineVec.cxx | 7 +- vtkm/exec/arg/CMakeLists.txt | 2 - vtkm/exec/arg/FetchTagArrayDirectOut.h | 38 ++++++++-- ...rrayDirectOutArrayHandleGroupVecVariable.h | 69 ------------------- ...TagArrayDirectOutArrayHandleRecombineVec.h | 46 ------------- 9 files changed, 82 insertions(+), 141 deletions(-) create mode 100644 docs/changelog/output-vec-like-fix.md delete mode 100644 vtkm/exec/arg/FetchTagArrayDirectOutArrayHandleGroupVecVariable.h delete mode 100644 vtkm/exec/arg/FetchTagArrayDirectOutArrayHandleRecombineVec.h diff --git a/docs/changelog/output-vec-like-fix.md b/docs/changelog/output-vec-like-fix.md new file mode 100644 index 000000000..6b32ccd43 --- /dev/null +++ b/docs/changelog/output-vec-like-fix.md @@ -0,0 +1,36 @@ +# Support using arrays with dynamic Vec-likes as output arrays + +When you use an `ArrayHandle` as an output array in a worklet (for example, +as a `FieldOut`), the fetch operation does not read values from the array +during the `Load`. Instead, it just constructs a new object. This makes +sense as an output array is expected to have garbage in it anyway. + +This is a problem for some special arrays that contain `Vec`-like objects +that are sized dynamically. For example, if you use an +`ArrayHandleGroupVecVariable`, each entry is a dynamically sized `Vec`. The +array is referenced by creating a special version of `Vec` that holds a +reference to the array portal and an index. Components are retrieved and +set by accessing the memory in the array portal. This allows us to have a +dynamically sized `Vec` in the execution environment without having to +allocate within the worklet. + +The problem comes when we want to use one of these arrays with `Vec`-like +objects for an output. The typical fetch fails because you cannot construct +one of these `Vec`-like objects without an array portal to bind it to. In +these cases, we need the fetch to create the `Vec`-like object by reading +it from the array. Even though the data will be garbage, you get the +necessary buffer into the array (and nothing more). + +Previously, the problem was fixed by creating partial specializations of +the `Fetch` for these `ArrayHandle`s. This worked OK as long as you were +using the array directly. However, the approach failed if the `ArrayHandle` +was wrapped in another `ArrayHandle` (for example, if an `ArrayHandleView` +was applied to an `ArrayHandleGroupVecVariable`). + +To get around this problem and simplify things, the basic `Fetch` for +direct output arrays is changed to handle all cases where the values in the +`ArrayHandle` cannot be directly constructed. A compile-time check of the +array's value type is checked with `std::is_default_constructible`. If it +can be constructed, then the array is not accessed. If it cannot be +constructed, then it grabs a value out of the array. + diff --git a/vtkm/VecFromPortal.h b/vtkm/VecFromPortal.h index 0a0381ca5..9906c0e61 100644 --- a/vtkm/VecFromPortal.h +++ b/vtkm/VecFromPortal.h @@ -31,14 +31,6 @@ class VecFromPortal public: using ComponentType = typename std::remove_const::type; - VTKM_SUPPRESS_EXEC_WARNINGS - VTKM_EXEC_CONT - VecFromPortal() - : NumComponents(0) - , Offset(0) - { - } - VTKM_SUPPRESS_EXEC_WARNINGS VTKM_EXEC_CONT VecFromPortal(const PortalType& portal, vtkm::IdComponent numComponents = 0, vtkm::Id offset = 0) diff --git a/vtkm/cont/ArrayHandleGroupVecVariable.h b/vtkm/cont/ArrayHandleGroupVecVariable.h index ed0401855..1c28ab194 100644 --- a/vtkm/cont/ArrayHandleGroupVecVariable.h +++ b/vtkm/cont/ArrayHandleGroupVecVariable.h @@ -314,10 +314,6 @@ make_ArrayHandleGroupVecVariable(const ComponentsArrayHandleType& componentsArra } } // namespace vtkm::cont -//============================================================================= -// Specializations of worklet arguments using ArrayHandleGropuVecVariable -#include - //============================================================================= // Specializations of serialization related classes /// @cond SERIALIZATION diff --git a/vtkm/cont/ArrayHandleRecombineVec.h b/vtkm/cont/ArrayHandleRecombineVec.h index 59d7e5755..14e971ee8 100644 --- a/vtkm/cont/ArrayHandleRecombineVec.h +++ b/vtkm/cont/ArrayHandleRecombineVec.h @@ -78,7 +78,14 @@ public: VTKM_EXEC_CONT RecombineVec& operator=(const RecombineVec& src) { - this->DoCopy(src); + if ((&this->Portals[0] != &src.Portals[0]) || (this->Index != src.Index)) + { + this->DoCopy(src); + } + else + { + // Copying to myself. Do not need to do anything. + } return *this; } @@ -648,8 +655,4 @@ struct ArrayExtractComponentImpl } } // namespace vtkm::cont -//============================================================================= -// Specializations of worklet arguments using ArrayHandleGropuVecVariable -#include - #endif //vtk_m_cont_ArrayHandleRecombineVec_h diff --git a/vtkm/cont/testing/UnitTestArrayHandleRecombineVec.cxx b/vtkm/cont/testing/UnitTestArrayHandleRecombineVec.cxx index 7bedc750b..d488948c0 100644 --- a/vtkm/cont/testing/UnitTestArrayHandleRecombineVec.cxx +++ b/vtkm/cont/testing/UnitTestArrayHandleRecombineVec.cxx @@ -10,6 +10,7 @@ #include +#include #include #include @@ -80,8 +81,12 @@ struct TestRecombineVecAsOutput vtkm::cont::Invoker invoke; invoke(PassThrough{}, baseArray, recombinedArray); - VTKM_TEST_ASSERT(test_equal_ArrayHandles(baseArray, outputArray)); + + // Try outputing to a recombine vec inside of another fancy ArrayHandle. + auto reverseOutput = vtkm::cont::make_ArrayHandleReverse(recombinedArray); + invoke(PassThrough{}, baseArray, reverseOutput); + VTKM_TEST_ASSERT(test_equal_ArrayHandles(baseArray, reverseOutput)); } }; diff --git a/vtkm/exec/arg/CMakeLists.txt b/vtkm/exec/arg/CMakeLists.txt index 92e774ead..036cbd97a 100644 --- a/vtkm/exec/arg/CMakeLists.txt +++ b/vtkm/exec/arg/CMakeLists.txt @@ -19,8 +19,6 @@ set(headers FetchTagArrayDirectIn.h FetchTagArrayDirectInOut.h FetchTagArrayDirectOut.h - FetchTagArrayDirectOutArrayHandleGroupVecVariable.h - FetchTagArrayDirectOutArrayHandleRecombineVec.h FetchTagArrayNeighborhoodIn.h FetchTagArrayTopologyMapIn.h FetchTagExecObject.h diff --git a/vtkm/exec/arg/FetchTagArrayDirectOut.h b/vtkm/exec/arg/FetchTagArrayDirectOut.h index 1ca3d997e..6cfd02d4a 100644 --- a/vtkm/exec/arg/FetchTagArrayDirectOut.h +++ b/vtkm/exec/arg/FetchTagArrayDirectOut.h @@ -13,6 +13,8 @@ #include #include +#include + namespace vtkm { namespace exec @@ -35,14 +37,15 @@ struct Fetch { + using ValueType = typename ExecObjectType::ValueType; + VTKM_SUPPRESS_EXEC_WARNINGS template - VTKM_EXEC auto Load(const ThreadIndicesType&, const ExecObjectType&) const -> - typename ExecObjectType::ValueType + VTKM_EXEC ValueType Load(const ThreadIndicesType& indices, + const ExecObjectType& arrayPortal) const { - // Load is a no-op for this fetch. - using ValueType = typename ExecObjectType::ValueType; - return ValueType(); + return this->DoLoad( + indices, arrayPortal, typename std::is_default_constructible::type{}); } VTKM_SUPPRESS_EXEC_WARNINGS @@ -51,9 +54,32 @@ struct Fetch(value)); } + +private: + VTKM_SUPPRESS_EXEC_WARNINGS + template + VTKM_EXEC ValueType DoLoad(const ThreadIndicesType&, const ExecObjectType&, std::true_type) const + { + // Load is a no-op for this fetch. + return ValueType(); + } + + VTKM_SUPPRESS_EXEC_WARNINGS + template + VTKM_EXEC ValueType DoLoad(const ThreadIndicesType& indices, + const ExecObjectType& arrayPortal, + std::false_type) const + { + // Cannot create a ValueType object, so pull one out of the array portal. This may seem + // weird because an output array often has garbage in it. However, this case can happen + // with special arrays with Vec-like values that reference back to the array memory. + // For example, with ArrayHandleRecombineVec, the values are actual objects that point + // back to the array for on demand reading and writing. You need the buffer established + // by the array even if there is garbage in that array. + return arrayPortal.Get(indices.GetOutputIndex()); + } }; } } diff --git a/vtkm/exec/arg/FetchTagArrayDirectOutArrayHandleGroupVecVariable.h b/vtkm/exec/arg/FetchTagArrayDirectOutArrayHandleGroupVecVariable.h deleted file mode 100644 index 72e953672..000000000 --- a/vtkm/exec/arg/FetchTagArrayDirectOutArrayHandleGroupVecVariable.h +++ /dev/null @@ -1,69 +0,0 @@ -//============================================================================ -// Copyright (c) Kitware, Inc. -// All rights reserved. -// See LICENSE.txt for details. -// -// This software is distributed WITHOUT ANY WARRANTY; without even -// the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR -// PURPOSE. See the above copyright notice for more information. -//============================================================================ -#ifndef vtk_m_exec_arg_FetchTagArrayDirectOutArrayHandleGroupVecVariable_h -#define vtk_m_exec_arg_FetchTagArrayDirectOutArrayHandleGroupVecVariable_h - -#include - -// We need to override the fetch for output fields using -// ArrayPortalGroupVecVariable because this portal does not behave like most -// ArrayPortals. Usually you ignore the Load and implement the Store. But if -// you ignore the Load, the VecFromPortal gets no portal to set values into. -// Instead, you need to implement the Load to point to the array portal. You -// can also ignore the Store because the data is already set in the array at -// that point. - -// This file is included from ArrayHandleGroupVecVariable.h - -namespace vtkm -{ -namespace exec -{ -namespace arg -{ - -// We need to override the fetch for output fields using -// ArrayPortalGroupVecVariable because this portal does not behave like most -// ArrayPortals. Usually you ignore the Load and implement the Store. But if -// you ignore the Load, the VecFromPortal gets no portal to set values into. -// Instead, you need to implement the Load to point to the array portal. You -// can also ignore the Store because the data is already set in the array at -// that point. -template -struct Fetch> -{ - using ExecObjectType = - vtkm::internal::ArrayPortalGroupVecVariable; - using ValueType = typename ExecObjectType::ValueType; - - VTKM_SUPPRESS_EXEC_WARNINGS - template - VTKM_EXEC ValueType Load(const ThreadIndicesType& indices, - const ExecObjectType& arrayPortal) const - { - return arrayPortal.Get(indices.GetOutputIndex()); - } - - VTKM_SUPPRESS_EXEC_WARNINGS - template - VTKM_EXEC void Store(const ThreadIndicesType&, const ExecObjectType&, const ValueType&) const - { - // We can actually ignore this because the VecFromPortal will already have - // set new values in the array. - } -}; - -} -} -} // namespace vtkm::exec::arg - -#endif //vtk_m_exec_arg_FetchTagArrayDirectOutArrayHandleGroupVecVariable_h diff --git a/vtkm/exec/arg/FetchTagArrayDirectOutArrayHandleRecombineVec.h b/vtkm/exec/arg/FetchTagArrayDirectOutArrayHandleRecombineVec.h deleted file mode 100644 index 1ad42b8e0..000000000 --- a/vtkm/exec/arg/FetchTagArrayDirectOutArrayHandleRecombineVec.h +++ /dev/null @@ -1,46 +0,0 @@ -//============================================================================ -// Copyright (c) Kitware, Inc. -// All rights reserved. -// See LICENSE.txt for details. -// -// This software is distributed WITHOUT ANY WARRANTY; without even -// the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR -// PURPOSE. See the above copyright notice for more information. -//============================================================================ -#ifndef vtk_m_exec_arg_FetchTagArrayDirectOutArrayHandleRecombineVec_h -#define vtk_m_exec_arg_FetchTagArrayDirectOutArrayHandleRecombineVec_h - -#include -#include - -// The `Fetch` for direct array out breaks for `ArrayHandleRecombineVec` because the `Load` -// method attempts to create a `vtkm::internal::RecombineVec` with a default constructor, -// which does not exist. Instead, have the direct out `Fetch` behave like the direct in/out -// `Fetch`, which loads the initial value from the array. The actual load will not load the -// data but rather set up the portals in the returned object, which is necessary for the -// later `Store` to work anyway. - -// This file is included from ArrayHandleRecombineVec.h - -namespace vtkm -{ -namespace exec -{ -namespace arg -{ - -template -struct Fetch> - : Fetch> -{ -}; - -} -} -} // namespace vtkm::exec::arg - -#endif //vtk_m_exec_arg_FetchTagArrayDirectOutArrayHandleRecombineVec_h