The `Variant` class was missing a way to check the type. You could do it
indirectly using `variant.GetIndex() == variant.GetIndexOf<T>()`, but
having this convenience function is more clear.
There was a bug where if you attempted to copy a variant that was not
valid (i.e. did not hold an object), a seg fault could happen. This has
been changed to set the target variant to also be invalid.
We have been doing a better job at hiding device code (and moving code
into libraries). Smoke out source that no longer needs to be compiled by
device compilers.
Previously, if you called `Get` on a `Variant` with a type that is not
in the list of types supported by the `Variant`, that would attempt to
look up the type at index `-1` and could spin the compiler into an
endless loop.
Instead, check for the case where you are attempting to get a type from
the `Variant` not listed in its templat arguments. In this case, instead
of producing a compiler error, produce a runtime error. Although this
increases the possibility that a bad compile path is being generated, it
simplifies creating templated code that produces cases we don't care
about.
Scheduling topology map workets for `CellSetExtrude` always worked, but
the there were indexing problems when a `Scatter` or a `Mask` was used.
This has been corrected, and now `Scatter`s and `Mask`s are supported on
topology maps on `CellSetExtrude`.
The reason why we did not support shared libraries when CUDA compiles
were on is that virtual methods require a special linking step to pull
together all virtual methods that might be called. I other words, you
cannot call a virtual CUDA method defined inside a library. This
requirement goes away when virtuals are removed.
Also removed the necessity of using seprable compilation with cuda.
Again, this is only needed when a CUDA function is defined in one
translation unit and used in another. Now we can enforce that all
translation units define their own CUDA functions.
Also, suppress warnings in cuda/internal/ExecutionPolicy.h
This is where we call the thrust algorithms. There must be some loop
where it, on some code path, calls back a host function. This must be in
an execution path that never happens. The thrust version has its own
suppress, but that does not seem to actually suppress the warning (it
just means that the warning does not tell you where the actual call is).
Get around the problem by suppressing the warnings in VTK-m.
Co-authored-by: Kenneth Moreland <morelandkd@ornl.gov>
Co-authored-by: Vicente Adolfo Bolea Sanchez <vicente.bolea@kitware.com>
Signed-off-by: Vicente Adolfo Bolea Sanchez <vicente.bolea@kitware.com>
The code in `vtkm/cont/Testing.h` now requires a library, which is not
built if testing is not built. Thus, the benchmarking code was giving a
compile error if benchmarking was on but testing was off.
Change the benchmarking to not rely on anything in the Testing
framework. This means using classes in `vtkm/source` instead of
`MakeTestData`. Also avoid using the `TestValue` defined for the tests.
(In one case, we have a simple replacement.) Also had to fix a problem
with a header file not defining everything it needed to compile.
Deprecate `VirtualObjectHandle` and all other classes that are used to
implement objects with virtual methods in the execution environment.
Additionally, the code is updated so that if the
`VTKm_NO_DEPRECATED_VIRTUAL` flag is set none of the code is compiled at
all. This opens us up to opportunities that do not work with virtual
methods such as backends that do not support virtual methods and dynamic
libraries for CUDA.
There appears to be a bug in CUDA 9.2 where if you have a class that
contains a struct that itself has to have padding in the middle for
alignment purposes and you then put that class in a union with other
classes, it seems like that padding can cause problems with other
objects in the union.
It always worked to trivially copy these classes, but the compiler did
not think so because copy constructors were defined. Change these
constructors to be default so that the compler can properly check
triviality.
Create a `VaraintUnion` that is an actual C++ `union` to store the data
in a `Variant`.
You may be asking yourself, why not just use an `std::aligned_union`
rather than a real union type? That was our first implementation, but
the problem is that the `std::aligned_union` reference needs to be
recast to the actual type. Typically you would do that with
`reinterpret_cast`. However, doing that leads to undefined behavior. The
C++ compiler assumes that 2 pointers of different types point to
different memory (even if it is clear that they are set to the same
address). That means optimizers can remove code because it "knows" that
data in one type cannot affect data in another type. To safely change
the type of an `std::aligned_union`, you really have to do an
`std::memcpy`. This is problematic for types that cannot be trivially
copied. Another problem is that we found that device compilers do not
optimize the memcpy as well as most CPU compilers. Likely, memcpy is
used much less frequently on GPU devices.
`std::is_trivial` is part of the C++14 specification. However, we have
encountered multiple compilers that purport to implement C++14 but do
not implement `std::is_trivial` and the like checks correctly.
To avoid such issues, only use `std::is_trivial` on compilers that we
have tested to support it.
`PointLocator`s have changed from being virtual objects to being trivial
objects. Part of this change means that when a worklet gets the
execution object for a point locator, it gets the actual object (or a
reference to it) instead of a pointer to an object. This means that the
new code uses the `.` operator to access the locator's features instead
of the `->` operator.
To support code still using the deprecated functionality, added a
specific `->` operator to the locator execution object to make it behave
as if it were a pointer. However, this operator is marked deprecated to
warn the user that they should modify their code to use the `.` instead.
Deprecated the `CellLocator` class and made all methods of the
other `CellLocator` classes non-virtual. General locators can
still use the `CellLocatorGeneral` class, but this class now
only works with a predefined set of locators. (The functionality
to provide a function to select a locator has been removed.)
0797359c5 Make ExecutionWholeArray objects not depend on device type
0bee74438 Support DeviceAdapterId in deprecated ArrayHandle
Acked-by: Kitware Robot <kwrobot@kitware.com>
Acked-by: Nick Thompson <nathompson7@protonmail.com>
Merge-request: !2405
With recent changes to `Arrayhandle`, the type for the associated array
portal is now the same across all devices. This means that almost all
exec objects no longer need to be specialized on the device types. Thus,
clean up the whole array exec objects to no longer need to be templated
on device.
With recent changes to `ArrayHandle`, the type for the associated array
portal is now the same across all devices. This means that almost all
exec objects no longer need to be specialized on the device types. Thus,
clean up the locator exec objects to no longer need to be templated on
device.
Somewhere during this edit I removed a header file that didn't strictly
need to be there. This caused me to have to add
```cpp
```
in several places in the code.
The newer version of `ArrayHandle` no longer supports different types of
portals for different devices. Thus, the `ReadPortalType` and
`WritePortalType` are sufficient for all types of portals across all
devices.
This significantly simplifies supporting execution objects on devices,
and thus this change also includes many changes to various execution
objects to remove their dependence on the device adapter tag.
Some (but not all) versions of visual studio seem to have a bug that
causes a syntax error when using the deprecated attribute in a templated
constructor.
The actual code for AtomicArrayExecutionObject does not need to be
specialized by the device. The functionality is implemented by calling
the vtkm::Atomic* methods, which are properly implemented on each
device.
Some of the special `ArrayHandle`s require specialized versions of
`vtkm::exec::arg::Fetch`. The specializations were not put in the
respective vtkm/exec/arg/Fetch*.h header files because the definition of
the `ArrayHandle`s was not available there. The implementation was in
the ArrayHandle*.h files, but it is hard to find the specialization
there.
Instead, make a secondary header file in vtkm/exec/arg that implements
the Fetch specialization and include it from the ArrayHandle*.h file.
That way, the basic Fetch does not have to include odd `ArrayHandle`
types but the `Fetch` implemenations are still all located together.
The `Variant` class has separate implementations for its move and copy
constructors/assignment operators depending on whether the classes it
holds can be trivially moved. If the objects are trivial, Variant is
trivial as well. However, in the case where the objects are not trivial,
special construction and copying needs to be done.
Previously, the non-trivial `Variant` defined a move constructor that
did a byte copy of the contained object and reset the right hand side
object so that it did not attempt to destroy the object. That usually
works because it guarantees that only one version of the `Variant` will
attempt to destroy the object and its resources should be cleaned up
correctly.
But C++ is a funny language that lets you do weird things. Turns out
there are cases where moving the location of memory for an object
without calling the proper copy method can invalidate the object. For
example, if the object holds a pointer to one of its own members, that
pointer will become invalid. Also, if it points to something that points
back, then the object will need to update those pointers when it is
moved. GCC's version of `std::string` seems to be a type like this.
Solve the problem by simply deleting the move constructors. The copy
constructors and destructor will be called instead to properly manage
the object. A test for these conditions is added to `UnitTestVariant`.
The `Variant` class is templated to hold objects of other types.
Depending on whether those objects of are meant to be used in the
control or execution side, the methods on `Variant` might need to be
declared with (or without) special modifiers. We can sometimes try to
compile the `Variant` methods for both host and device and ask the
device compiler to ignore incompatibilities, but that does not always
work.
To get around that, create two different implementations of `Variant`.
Their API and implementation is exactly the same except one declares its
methods with `VTKM_CONT` and the other its methods `VTKM_EXEC`.
The old atomic compare and swap operations (`vtkm::AtomicCompareAndSwap`
and `vtkm::exec::AtomicArrayExecutionObject::CompareAndSwap`) had an
order of arguments that was confusing. The order of the arguments was
shared pointer (or index), desired value, expected value. Most people
probably assume expected value comes before desired value. And this
order conflicts with the order in the `std` methods, GCC atomics, and
Kokkos.
Change the interface of atomic operations to be patterned off the
`std::atomic_compare_exchange` and `std::atomic<T>::compare_exchange`
methods. First, these methods have a more intuitive order of parameters
(shared pointer, expected, desired). Second, rather than take a value
for the expected and return the actual old value, they take a pointer to
the expected value (or reference in `AtomicArrayExecutionObject`) and
modify this value in the case that it does not match the actual value.
This makes it harder to mix up the expected and desired parameters.
Also, because the methods return a bool indicating whether the value was
changed, there is an additional benefit that compare-exchange loops are
implemented easier.
For example, consider you want to apply the function `MyOp` on a
`sharedValue` atomically. With the old interface, you would have to do
something like this.
```cpp
T oldValue;
T newValue;
do
{
oldValue = *sharedValue;
newValue = MyOp(oldValue);
} while (vtkm::AtomicCompareAndSwap(sharedValue, newValue, oldValue) != oldValue);
```
With the new interface, this is simplfied to this.
```cpp
T oldValue = *sharedValue;
while (!vtkm::AtomicCompareExchange(sharedValue, &oldValue, MyOp(oldValue));
```
Previously vtk-m allowed users to issue atomic loads on constant
values which is problematic for the following reasons:
- can be a source of undefined behavior
- not supported by kokkos
This issue was detected when using kokkos HIP atomic implementation
The new name reflects better what the underlying algorithm does. It also
helps prevent confusion about what types of data the locator is good
for. The old name suggested it only worked for structured grids, which
is not the case.
