From 0182eb9d9d973028f11327cf1c5c8c2b6c83ab73 Mon Sep 17 00:00:00 2001 From: Kenneth Moreland Date: Tue, 4 Apr 2023 17:14:38 -0600 Subject: [PATCH] Test variant arguments to worklets Add a regression test for passing a variant as an argument to a worklet. Variants are tricky objects, and the compiler might make some strange assumptions during optimization. One case that popped up in particular was when a variant contained two objects with the same `sizeof` but the first object had padding. When the variant contained the second object, the value under the padded part of the first object was not set. --- vtkm/internal/VariantImplDetail.h | 2 +- vtkm/internal/VariantImplDetail.h.in | 2 +- .../UnitTestWorkletMapFieldExecArg.cxx | 187 +++++++++++++++++- 3 files changed, 185 insertions(+), 6 deletions(-) diff --git a/vtkm/internal/VariantImplDetail.h b/vtkm/internal/VariantImplDetail.h index 712b35f1c..ce2258948 100644 --- a/vtkm/internal/VariantImplDetail.h +++ b/vtkm/internal/VariantImplDetail.h @@ -75,7 +75,7 @@ constexpr std::size_t MaxSizeOf() #endif // -------------------------------------------------------------------------------- -// Placeholder for a fullly used structure of the given type. +// Placeholder for a fully used structure of the given type. template 8)> struct SizedPlaceholder { diff --git a/vtkm/internal/VariantImplDetail.h.in b/vtkm/internal/VariantImplDetail.h.in index bdca13e8c..4628603a5 100644 --- a/vtkm/internal/VariantImplDetail.h.in +++ b/vtkm/internal/VariantImplDetail.h.in @@ -108,7 +108,7 @@ constexpr std::size_t MaxSizeOf() #endif // -------------------------------------------------------------------------------- -// Placeholder for a fullly used structure of the given type. +// Placeholder for a fully used structure of the given type. template 8)> struct SizedPlaceholder { diff --git a/vtkm/worklet/testing/UnitTestWorkletMapFieldExecArg.cxx b/vtkm/worklet/testing/UnitTestWorkletMapFieldExecArg.cxx index 38b302d12..25f5c93fa 100644 --- a/vtkm/worklet/testing/UnitTestWorkletMapFieldExecArg.cxx +++ b/vtkm/worklet/testing/UnitTestWorkletMapFieldExecArg.cxx @@ -10,13 +10,19 @@ #include #include #include +#include #include #include #include +#include + #include +namespace map_exec_field +{ + struct SimpleExecObject : vtkm::cont::ExecutionObjectBase { template @@ -55,9 +61,6 @@ struct TestExecObjectWorklet }; }; -namespace map_exec_field -{ - static constexpr vtkm::Id ARRAY_SIZE = 10; template @@ -105,6 +108,179 @@ struct DoTestWorklet } }; +struct StructWithPadding +{ + vtkm::Int32 A; + // Padding here + vtkm::Int64 C; +}; + +struct StructWithoutPadding +{ + vtkm::Int32 A; + vtkm::Int32 B; + vtkm::Int64 C; +}; + +struct LargerStruct +{ + vtkm::Int64 C; + vtkm::Int64 D; + vtkm::Int64 E; +}; + +using VariantTypePadding = vtkm::exec::Variant; +using VariantTypeSizes = vtkm::exec::Variant; + +struct VarientPaddingExecObj : vtkm::cont::ExecutionObjectBase +{ + VariantTypePadding Variant; + VTKM_CONT VariantTypePadding PrepareForExecution(const vtkm::cont::DeviceAdapterId&, + vtkm::cont::Token&) const + { + return this->Variant; + } +}; +struct VarientSizesExecObj : vtkm::cont::ExecutionObjectBase +{ + VariantTypeSizes Variant; + VTKM_CONT VariantTypeSizes PrepareForExecution(const vtkm::cont::DeviceAdapterId&, + vtkm::cont::Token&) const + { + return this->Variant; + } +}; + +struct TestVariantExecObjectPadding : vtkm::worklet::WorkletMapField +{ + using ControlSignature = void(FieldOut a, FieldOut c, ExecObject varient); + // Using an output field as the domain is weird, but it works. + using InputDomain = _1; + + VTKM_EXEC void operator()(vtkm::Int32& a, vtkm::Int64& c, const VariantTypePadding& variant) const + { + a = variant.Get().A; + c = variant.Get().C; + } +}; + +struct TestVariantExecObjectNoPadding : vtkm::worklet::WorkletMapField +{ + using ControlSignature = void(FieldOut a, FieldOut b, FieldOut c, ExecObject varient); + // Using an output field as the domain is weird, but it works. + using InputDomain = _1; + + VTKM_EXEC void operator()(vtkm::Int32& a, + vtkm::Int32& b, + vtkm::Int64& c, + const VariantTypePadding& variant) const + { + a = variant.Get().A; + b = variant.Get().B; + c = variant.Get().C; + } +}; + +struct TestVariantExecObjectLarger : vtkm::worklet::WorkletMapField +{ + using ControlSignature = void(FieldOut c, FieldOut d, FieldOut e, ExecObject varient); + // Using an output field as the domain is weird, but it works. + using InputDomain = _1; + + VTKM_EXEC void operator()(vtkm::Int64& c, + vtkm::Int64& d, + vtkm::Int64& e, + const VariantTypeSizes& variant) const + { + c = variant.Get().C; + d = variant.Get().D; + e = variant.Get().E; + } +}; + +void DoTestVariant() +{ + vtkm::cont::ArrayHandle a; + vtkm::cont::ArrayHandle b; + vtkm::cont::ArrayHandle c; + vtkm::cont::ArrayHandle d; + vtkm::cont::ArrayHandle e; + + // Usually you don't need to allocate output arrays, but these worklets do a + // weird thing of using an output array as the input domain (because the + // generative worklets have no input). It's weird to use an output field as + // the input domain, but it works as long as you preallocate the data. + a.Allocate(ARRAY_SIZE); + b.Allocate(ARRAY_SIZE); + c.Allocate(ARRAY_SIZE); + d.Allocate(ARRAY_SIZE); + e.Allocate(ARRAY_SIZE); + + vtkm::cont::Invoker invoke; + + std::cout << "Struct with Padding" << std::endl; + { + VarientPaddingExecObj execObject; + execObject.Variant = + StructWithPadding{ TestValue(0, vtkm::Int32{}), TestValue(1, vtkm::Int64{}) }; + invoke(TestVariantExecObjectPadding{}, a, c, execObject); + auto aPortal = a.ReadPortal(); + auto cPortal = c.ReadPortal(); + for (vtkm::Id index = 0; index < ARRAY_SIZE; ++index) + { + VTKM_TEST_ASSERT(aPortal.Get(index) == TestValue(0, vtkm::Int32{})); + VTKM_TEST_ASSERT(cPortal.Get(index) == TestValue(1, vtkm::Int64{})); + } + } + + std::cout << "Struct without Padding" << std::endl; + { + VarientPaddingExecObj execObject; + execObject.Variant = StructWithoutPadding{ TestValue(2, vtkm::Int32{}), + TestValue(3, vtkm::Int32{}), + TestValue(4, vtkm::Int64{}) }; + invoke(TestVariantExecObjectNoPadding{}, a, b, c, execObject); + auto aPortal = a.ReadPortal(); + auto bPortal = b.ReadPortal(); + auto cPortal = c.ReadPortal(); + // An odd bug was observed with some specific compilers. (Specifically, this was + // last observed with GCC5 used with nvcc compiling CUDA code for the Pascal + // architecture.) It concerned a Variant that contained 2 or more objects of the + // same `sizeof` and the first one listed had some padding (to satisfy alignment) + // and the second one did not. Internally, the `Variant` object constructs a + // `union` of types in the order listed. The compiler seemed to recognize that the + // first entry in the union was the "largest" and used that for trivial copies. + // However, it also recognized the padding in that first object and skipped + // copying that value even if the union was set to the second object. If that + // condition is happening, you will probably see a failure when testing the + // bPortal below. + for (vtkm::Id index = 0; index < ARRAY_SIZE; ++index) + { + VTKM_TEST_ASSERT(aPortal.Get(index) == TestValue(2, vtkm::Int32{})); + VTKM_TEST_ASSERT(bPortal.Get(index) == TestValue(3, vtkm::Int32{})); + VTKM_TEST_ASSERT(cPortal.Get(index) == TestValue(4, vtkm::Int64{})); + } + } + + std::cout << "LargerStruct" << std::endl; + { + VarientSizesExecObj execObject; + execObject.Variant = LargerStruct{ TestValue(5, vtkm::Int64{}), + TestValue(6, vtkm::Int64{}), + TestValue(7, vtkm::Int64{}) }; + invoke(TestVariantExecObjectLarger{}, c, d, e, execObject); + auto cPortal = c.ReadPortal(); + auto dPortal = d.ReadPortal(); + auto ePortal = e.ReadPortal(); + for (vtkm::Id index = 0; index < ARRAY_SIZE; ++index) + { + VTKM_TEST_ASSERT(cPortal.Get(index) == TestValue(5, vtkm::Int64{})); + VTKM_TEST_ASSERT(dPortal.Get(index) == TestValue(6, vtkm::Int64{})); + VTKM_TEST_ASSERT(ePortal.Get(index) == TestValue(7, vtkm::Int64{})); + } + } +} + void TestWorkletMapFieldExecArg(vtkm::cont::DeviceAdapterId id) { std::cout << "Testing Worklet with WholeArray on device adapter: " << id.GetName() << std::endl; @@ -112,9 +288,12 @@ void TestWorkletMapFieldExecArg(vtkm::cont::DeviceAdapterId id) std::cout << "--- Worklet accepting all types." << std::endl; vtkm::testing::Testing::TryTypes(map_exec_field::DoTestWorklet(), vtkm::TypeListCommon()); + + std::cout << "--- Worklet passing variant." << std::endl; + DoTestVariant(); } -} // anonymous namespace +} // anonymous-ish namespace int UnitTestWorkletMapFieldExecArg(int argc, char* argv[]) {