Introduce PairValueIndex and Experimental::attach_indices #969

aprokop · 2023-11-03T15:37:01Z

There are two use cases that are currently mixed when using LegacyValues:

Legacy classes use LegacyValues to pass to the new interface
User wants to attach indices to the passed primitives without
constructing them themselves

The difference is that 1) should not be exposed to a user, while 2) is intended to.

With the introduction of the RangeTraits in the future, the use cases diverge: 1) will still use AccessTraits for backwards compatibility, while 2) will switch to using RangeTraits.

This patch separates the two. They still share a lot of commonalities at the moment, but that will change with RangeTraits.

Additional changes:

Swap value-index order
The reason being is that it is easer to align an index (likely 4-byte int) than a value (which could be a 12-byte point, 24-byte box, or whatever).

aprokop · 2023-12-07T15:39:56Z

This used to improve performance by itself. However, I no longer see any changes after #976 was merged. So now it is purely about interface and not performance.

src/details/ArborX_PairValueIndex.hpp

aprokop · 2023-12-07T17:52:15Z

@dalg24 Appreciate the feedback, addressed it.

dalg24 · 2023-12-07T18:23:44Z

examples/triangle_intersection/triangle_intersection.cpp

      tree(execution_space,
-           ArborX::Details::LegacyValues<decltype(triangles), Box>{triangles});
+           ArborX::Details::AttachIndices<decltype(triangles)>{triangles});


OK but noting this still bad to advertise using private implementation details in an example.
I am not expecting you to address this in this PR.

AttachIndices should really be public. I was not sure whether to do this in this PR. If we agree on the name, I'll do it.

src/details/ArborX_DetailsNode.hpp

dalg24 · 2023-12-07T18:32:10Z

src/details/ArborX_PairValueIndex.hpp

+
+template <typename Value, typename Index = unsigned>
+struct PairValueIndex
+{


Is the intent that Value and Index can be deduced via decltype or should we define value_type and index_type?

So far, we don't have a need for either.

Well yes L50 but I suppose you got to the next comment and understand now

dalg24 · 2023-12-07T18:32:53Z

src/details/ArborX_DetailsLegacy.hpp

    }
    else
    {
      BoundingVolume bounding_volume{};
      expand(bounding_volume, Access::get(_primitives, i));
-      return value_type{(unsigned)i, bounding_volume};
+      return value_type{bounding_volume, (unsigned)i};


Wondering whether you should specify the trailing integral template parameter L29 or should we be deducing the right integral type to cast i to?

Good point. It is safer to specify the template arg on L29, as this is what it was hardcoded in APIv1.

src/details/ArborX_PairValueIndex.hpp

aprokop · 2023-12-08T12:44:16Z

CUDA 11.0.3 strikes again :(

aprokop · 2023-12-20T13:49:58Z

Rebased on master to solve conflicts after #970 merge.

dalg24 · 2023-12-20T15:15:30Z

benchmarks/brute_force_vs_bvh/brute_force_vs_bvh_timpl.hpp

-          bvh{space, ArborX::Details::LegacyValues<decltype(primitives), Point>{
-                         primitives}};
+                                      ArborX::PairValueIndex<Point>>
+          bvh{space, ArborX::AttachIndices<decltype(primitives)>{primitives}};


Any reason why you didn't rely on CTAD?

Suggested change

bvh{space, ArborX::AttachIndices<decltype(primitives)>{primitives}};

bvh{space, ArborX::AttachIndices{primitives}};

same comment everywhere

Yes. Annoyingly, CTAD fails for CUDA 11.0.3.

There are two use cases that are currently mixed when using LegacyValues: 1) Legacy classes use LegacyValues to pass to the new interface 2) User wants to attach indices to the passed primitives without constructing them themselves The difference is that 1) should not be exposed to a user, while 2) is intended to. With the introduction of the RangeTraits in the future, the use cases diverge: 1) will still use AccessTraits for backwards compatibility, while 2) will switch to using RangeTraits. This patch separates the two. They still share a lot of commonalities at the moment, but that will change with RangeTraits. In addition, the order is swapped between value and index in the struct. The reason being is that it is easer to align an index (likely 4-byte int) than a value (which could be a 12-byte point, 24-byte box, or whatever).

dalg24

I would probably prefer having an attach_indices function template that returns a non specified type. That way template argument deduction will work and it give us more freedom.

aprokop · 2023-12-22T18:57:14Z

We agreed to add Experimental::attach_indices, but keep ArborX::PairValueIndex. Will fix that.

aprokop · 2023-12-23T01:32:21Z

All builds pass but SYCL. A bunch of SYCL tests failed like

PI CUDA ERROR:
	Value:           700
	Name:            CUDA_ERROR_ILLEGAL_ADDRESS
	Description:     an illegal memory access was encountered
	Function:        cuda_piextUSMEnqueueMemcpy
	Source Location: /root/intel-llvm-mirror/sycl/plugins/cuda/pi_cuda.cpp:5023


PI CUDA ERROR:
	Value:           700
	Name:            CUDA_ERROR_ILLEGAL_ADDRESS
	Description:     an illegal memory access was encountered
	Function:        operator()
	Source Location: /root/intel-llvm-mirror/sycl/plugins/cuda/pi_cuda.cpp:2553

terminate called after throwing an instance of 'std::runtime_error'
  what():  There was a synchronous SYCL error:
Native API failed. Native API returns: -999 (Unknown PI error) -999 (Unknown PI error)
unknown location(0): fatal error: in "half_traversal<Kokkos__Device<Kokkos__Experimental__SYCL_ Kokkos__Experimental__SYCLDeviceUSMSpace>>": signal: SIGABRT (application abort requested)
/var/jenkins/workspace/ArborX_PR-969/test/tstDetailsHalfTraversal.cpp(60): last checkpoint: "half_traversal" test entry

aprokop · 2023-12-23T01:50:26Z

Actually, SYCL build has been broken since #973. So, it's not this PR's fault. We just had problems running anything in CI.

aprokop added API User visible interface modifications refactoring Code reorganization labels Nov 3, 2023

aprokop force-pushed the pair_index_value branch from e90e620 to 30a4ded Compare December 6, 2023 20:21

aprokop mentioned this pull request Dec 7, 2023

Cut down on the number of AccessTraits #967

Merged

aprokop force-pushed the pair_index_value branch from 30a4ded to 7c8ae03 Compare December 7, 2023 15:05

aprokop marked this pull request as ready for review December 7, 2023 15:06

aprokop force-pushed the pair_index_value branch from 7c8ae03 to 615c70b Compare December 7, 2023 15:09

dalg24 reviewed Dec 7, 2023

View reviewed changes

src/details/ArborX_PairValueIndex.hpp Outdated Show resolved Hide resolved

dalg24 reviewed Dec 7, 2023

View reviewed changes

src/details/ArborX_PairValueIndex.hpp Outdated Show resolved Hide resolved

dalg24 reviewed Dec 7, 2023

View reviewed changes

aprokop force-pushed the pair_index_value branch from 509fa76 to f4834ce Compare December 7, 2023 18:33

dalg24 reviewed Dec 7, 2023

View reviewed changes

src/details/ArborX_PairValueIndex.hpp Outdated Show resolved Hide resolved

aprokop force-pushed the pair_index_value branch from f4834ce to bd275da Compare December 7, 2023 19:13

aprokop mentioned this pull request Dec 20, 2023

API v2 tracker #872

Open

aprokop force-pushed the pair_index_value branch from bcf4f44 to 07b06c1 Compare December 20, 2023 13:49

dalg24 reviewed Dec 20, 2023

View reviewed changes

aprokop force-pushed the pair_index_value branch from 07b06c1 to 68e820e Compare December 21, 2023 16:54

aprokop added 2 commits December 21, 2023 12:16

Fix MLS

bf94eb5

aprokop force-pushed the pair_index_value branch from 68e820e to bf94eb5 Compare December 21, 2023 17:16

dalg24 approved these changes Dec 22, 2023

View reviewed changes

Move AttachIndices to Experimental and add attach_indices

2f317e6

aprokop force-pushed the pair_index_value branch from 7c76a15 to 2f317e6 Compare December 23, 2023 00:23

aprokop merged commit 8918428 into arborx:master Dec 23, 2023
1 of 2 checks passed

aprokop deleted the pair_index_value branch December 23, 2023 01:50

aprokop changed the title ~~Introduce PairValueIndex and AttachIndices~~ Introduce PairValueIndex and Experimental::attach_indices Dec 23, 2023

This was referenced Dec 23, 2023

Changelog 1.6 #985

Closed

Fix missing tag within AttachIndices #995

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce PairValueIndex and Experimental::attach_indices #969

Introduce PairValueIndex and Experimental::attach_indices #969

aprokop commented Nov 3, 2023 •

edited

Loading

aprokop commented Dec 7, 2023 •

edited

Loading

aprokop commented Dec 7, 2023

dalg24 Dec 7, 2023

aprokop Dec 7, 2023

dalg24 Dec 7, 2023

aprokop Dec 7, 2023

dalg24 Dec 7, 2023

dalg24 Dec 7, 2023

aprokop Dec 7, 2023

aprokop commented Dec 8, 2023

aprokop commented Dec 20, 2023

dalg24 Dec 20, 2023

aprokop Dec 20, 2023 •

edited

Loading

dalg24 left a comment

aprokop commented Dec 22, 2023

aprokop commented Dec 23, 2023 •

edited

Loading

aprokop commented Dec 23, 2023 •

edited

Loading

	bvh{space, ArborX::AttachIndices<decltype(primitives)>{primitives}};
	bvh{space, ArborX::AttachIndices{primitives}};

Introduce PairValueIndex and Experimental::attach_indices #969

Introduce PairValueIndex and Experimental::attach_indices #969

Conversation

aprokop commented Nov 3, 2023 • edited Loading

aprokop commented Dec 7, 2023 • edited Loading

aprokop commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aprokop commented Dec 8, 2023

aprokop commented Dec 20, 2023

Choose a reason for hiding this comment

aprokop Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

dalg24 left a comment

Choose a reason for hiding this comment

aprokop commented Dec 22, 2023

aprokop commented Dec 23, 2023 • edited Loading

aprokop commented Dec 23, 2023 • edited Loading

aprokop commented Nov 3, 2023 •

edited

Loading

aprokop commented Dec 7, 2023 •

edited

Loading

aprokop Dec 20, 2023 •

edited

Loading

aprokop commented Dec 23, 2023 •

edited

Loading

aprokop commented Dec 23, 2023 •

edited

Loading