Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove requirement on knowing geometry types in IndexableGetter #1211

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Remove requirement on knowing geometry types in IndexableGetter #1211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d450228
Revert "Remove is_pair_v"
aprokop Feb 4, 2025
ca28148
Remove requirement on knowing geometry types in IndexableGetter
aprokop Feb 4, 2025
6ffdcf2
Add compile-only test for bvh on custom type
aprokop Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions src/spatial/detail/ArborX_IndexableGetter.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,29 +27,30 @@ struct DefaultIndexableGetter
KOKKOS_DEFAULTED_FUNCTION
DefaultIndexableGetter() = default;

template <typename Geometry, typename Enable = std::enable_if_t<
GeometryTraits::is_valid_geometry<Geometry>>>
template <typename Geometry>
KOKKOS_FUNCTION auto const &operator()(Geometry const &geometry) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one will never be called
https://godbolt.org/z/64WGdP3n9

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is. For example, when running ArborX_Test_Clustering.exe, the stack leading to the call is:

    frame #5: 0x00000001006cbd24 ArborX_Test_Clustering.exe`auto const& ArborX::Experimental::DefaultIndexableGetter::operator(this=0x000000016fdf7648, geometry=0x0000611000076cc0)<ArborX::Point<3, float>>(ArborX::Point<3, float> const&) const at ArborX_IndexableGetter.hpp:34:5
    frame #6: 0x00000001006cbc80 ArborX_Test_Clustering.exe`ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter>::operator()(this=0x000000016fdf7630, i=0) const at ArborX_IndexableGetter.hpp:76:12
    frame #7: 0x00000001006cbadc ArborX_Test_Clustering.exe`void ArborX::Details::TreeConstruction::calculateBoundingBoxOfTheScene<Kokkos::Serial, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter>, ArborX::Box<3, float>>(this=0x000000016fdf7630, i=0, update=0x000000016fdf8660)::'lambda'(int, ArborX::Box<3, float>&)::operator()(int, ArborX::Box<3, float>&) const at ArborX_TreeConstruction.hpp:36:24
    frame #8: 0x00000001006cb918 ArborX_Test_Clustering.exe`std::__1::enable_if<std::is_void_v<Kokkos::Serial>, void>::type Kokkos::Impl::ParallelReduce<Kokkos::Impl::CombinedFunctorReducer<void ArborX::Details::TreeConstruction::calculateBoundingBoxOfTheScene<Kokkos::Serial, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter>, ArborX::Box<3, float>>(Kokkos::Serial const&, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter> const&, ArborX::Box<3, float>&)::'lambda'(int, ArborX::Box<3, float>&), Kokkos::Impl::FunctorAnalysis<Kokkos::Impl::FunctorPatternInterface::REDUCE, Kokkos::RangePolicy<>, ArborX::Details::GeometryReducer<ArborX::Box<3, float>, Kokkos::HostSpace>, ArborX::Box<3, float>>::Reducer, void>, Kokkos::RangePolicy<>, Kokkos::Serial>::exec<void>(this=0x000000016fdf7630, update=0x000000016fdf8660) const at Kokkos_Serial_Parallel_Range.hpp:91:7
    frame #9: 0x00000001006ca040 ArborX_Test_Clustering.exe`Kokkos::Impl::ParallelReduce<Kokkos::Impl::CombinedFunctorReducer<void ArborX::Details::TreeConstruction::calculateBoundingBoxOfTheScene<Kokkos::Serial, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter>, ArborX::Box<3, float>>(Kokkos::Serial const&, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter> const&, ArborX::Box<3, float>&)::'lambda'(int, ArborX::Box<3, float>&), Kokkos::Impl::FunctorAnalysis<Kokkos::Impl::FunctorPatternInterface::REDUCE, Kokkos::RangePolicy<>, ArborX::Details::GeometryReducer<ArborX::Box<3, float>, Kokkos::HostSpace>, ArborX::Box<3, float>>::Reducer, void>, Kokkos::RangePolicy<>, Kokkos::Serial>::execute(this=0x000000016fdf7630) const at Kokkos_Serial_Parallel_Range.hpp:137:20
    frame #10: 0x00000001006c8b30 ArborX_Test_Clustering.exe`Kokkos::Impl::ParallelReduceAdaptor<Kokkos::RangePolicy<>, void ArborX::Details::TreeConstruction::calculateBoundingBoxOfTheScene<Kokkos::Serial, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter>, ArborX::Box<3, float>>(Kokkos::Serial const&, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter> const&, ArborX::Box<3, float>&)::'lambda'(int, ArborX::Box<3, float>&), ArborX::Details::GeometryReducer<ArborX::Box<3, float>, Kokkos::HostSpace>>::execute_impl(label="ArborX::TreeConstruction::calculate_bounding_box_of_the_scene", policy=0x000000016fdf7c20, functor=0x000000016fdf7c70, return_value=0x000000016fdf79e0) at Kokkos_Parallel_Reduce.hpp:1539:13
    frame #11: 0x00000001006c850c ArborX_Test_Clustering.exe`std::__1::enable_if<!(Kokkos::Impl::ParallelReduceAdaptor<Kokkos::RangePolicy<>, void ArborX::Details::TreeConstruction::calculateBoundingBoxOfTheScene<Kokkos::Serial, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter>, ArborX::Box<3, float>>(Kokkos::Serial const&, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter> const&, ArborX::Box<3, float>&)::'lambda'(int, ArborX::Box<3, float>&), ArborX::Details::GeometryReducer<ArborX::Box<3, float>, Kokkos::HostSpace>>::is_array_reduction && std::is_pointer_v<Kokkos::Serial>), void>::type Kokkos::Impl::ParallelReduceAdaptor<Kokkos::RangePolicy<>, void ArborX::Details::TreeConstruction::calculateBoundingBoxOfTheScene<Kokkos::Serial, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter>, ArborX::Box<3, float>>(Kokkos::Serial const&, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter> const&, ArborX::Box<3, float>&)::'lambda'(int, ArborX::Box<3, float>&), ArborX::Details::GeometryReducer<ArborX::Box<3, float>, Kokkos::HostSpace>>::execute<ArborX::Details::GeometryReducer<ArborX::Box<3, float>, Kokkos::HostSpace>>(label="ArborX::TreeConstruction::calculate_bounding_box_of_the_scene", policy=0x000000016fdf7c20, functor=0x000000016fdf7c70, return_value=0x000000016fdf79e0) at Kokkos_Parallel_Reduce.hpp:1555:5
    frame #12: 0x00000001006c7f08 ArborX_Test_Clustering.exe`std::__1::enable_if<Kokkos::is_execution_policy<Kokkos::Serial>::value && (Kokkos::is_view<ArborX::Box<3, float>>::value || Kokkos::is_reducer<ArborX::Box<3, float>>::value || std::is_pointer_v<ArborX::Box<3, float>>), void>::type Kokkos::parallel_reduce<Kokkos::RangePolicy<>, void ArborX::Details::TreeConstruction::calculateBoundingBoxOfTheScene<Kokkos::Serial, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter>, ArborX::Box<3, float>>(Kokkos::Serial const&, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter> const&, ArborX::Box<3, float>&)::'lambda'(int, ArborX::Box<3, float>&), ArborX::Details::GeometryReducer<ArborX::Box<3, float>, Kokkos::HostSpace>>(label="ArborX::TreeConstruction::calculate_bounding_box_of_the_scene", policy=0x000000016fdf7c20, functor=0x000000016fdf7c70, return_value=0x000000016fdf7cb0) at Kokkos_Parallel_Reduce.hpp:1774:3
    frame #13: 0x00000001006a5f50 ArborX_Test_Clustering.exe`void ArborX::Details::TreeConstruction::calculateBoundingBoxOfTheScene<Kokkos::Serial, ArborX::Details::Indexables<ArborX::Details::AccessValuesI<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>>, ArborX::Experimental::DefaultIndexableGetter>, ArborX::Box<3, float>>(space=0x000000016fdfa300, indexables=0x000000016fdf86a0, scene_bounding_box=0x000000016fdf8660) at ArborX_TreeConstruction.hpp:31:3
    frame #14: 0x000000010045637c ArborX_Test_Clustering.exe`Kokkos::View<int*, ArborX::AccessTraits<HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>, void>::memory_space> ArborX::dbscan<Kokkos::Serial, HiddenView<Kokkos::View<ArborX::Point<3, float>*, Kokkos::Device<Kokkos::Serial, Kokkos::HostSpace>>>, float>(exec_space=0x000000016fdfa300, primitives=0x000000016fdfa950, eps=1.66068161, core_min_size=2, parameters=0x000000016fdfa360) at ArborX_DBSCAN.hpp:311:5

In your code, if you introduce an intermediary function goo that it goes through, it will use const& variant: https://godbolt.org/z/Krrdv39PP

{
return geometry;
}

template <typename Geometry, typename Enable = std::enable_if_t<
GeometryTraits::is_valid_geometry<Geometry>>>
template <typename Geometry,
typename Enable = std::enable_if_t<
!Details::is_pair_value_index_v<std::decay_t<Geometry>>>>
Comment on lines +37 to +38
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed so that universal reference does not grab PairValueIndex specializations.

KOKKOS_FUNCTION auto operator()(Geometry &&geometry) const
{
return geometry;
}

template <typename Value, typename Index>
KOKKOS_FUNCTION Value const &
operator()(PairValueIndex<Value, Index> const &pair) const
template <typename Geometry, typename Index>
KOKKOS_FUNCTION Geometry const &
operator()(PairValueIndex<Geometry, Index> const &pair) const
{
return pair.value;
}

template <typename Value, typename Index>
KOKKOS_FUNCTION Value operator()(PairValueIndex<Value, Index> &&pair) const
template <typename Geometry, typename Index>
KOKKOS_FUNCTION Geometry
operator()(PairValueIndex<Geometry, Index> &&pair) const
{
return pair.value;
dalg24 marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down
15 changes: 15 additions & 0 deletions src/spatial/detail/ArborX_PairValueIndex.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,21 @@ struct PairValueIndex
Index index;
};

namespace Details
{
template <typename T>
struct is_pair_value_index : public std::false_type
{};

template <typename Value, typename Index>
struct is_pair_value_index<PairValueIndex<Value, Index>> : public std::true_type
dalg24 marked this conversation as resolved.
Show resolved Hide resolved
{};

template <typename T>
inline constexpr bool is_pair_value_index_v = is_pair_value_index<T>::value;

} // namespace Details

} // namespace ArborX

#endif
34 changes: 34 additions & 0 deletions test/tstCompileOnlyTypeRequirements.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,37 @@ void check_bounding_volume_and_predicate_geometry_type_requirements()
KOKKOS_LAMBDA(NearestPredicate, auto){});
#endif
}

namespace Test
{

// clang-format off
struct FakePrimitiveGeometry {};

KOKKOS_FUNCTION void expand(ArborX::Box<3> &, FakePrimitiveGeometry) {}
KOKKOS_FUNCTION ArborX::Point<3> returnCentroid(FakePrimitiveGeometry) { return {}; }
// clang-format on

} // namespace Test

template <>
struct ArborX::GeometryTraits::dimension<Test::FakePrimitiveGeometry>
{
static constexpr int value = 3;
};
template <>
struct ArborX::GeometryTraits::coordinate_type<Test::FakePrimitiveGeometry>
{
using type = float;
};

// Compile-only
void check_hierarchy_for_custom_types()
{
using ExecutionSpace = Kokkos::DefaultExecutionSpace;
using MemorySpace = ExecutionSpace::memory_space;

Kokkos::View<Test::FakePrimitiveGeometry *, MemorySpace> primitives(
"primitives", 0);
ArborX::BoundingVolumeHierarchy tree(ExecutionSpace{}, primitives);
}