Add single-value access functions for array access

[upsj]

This adds bounds-checked array::load_value(size_type) and array::store_value(size_type, T) functions and uses them where possible

[MarcelKoch]

More places to replace:

• common/unified/matrix/dense_kernels.template.cpp:281
• core/baser/index_set.cpp:84
• core/factorization/par_ict.cpp:170|189|191
• core/matrix/sparsity_csr.cpp:136
• dpcpp/matrix/csr_kernels.dp.cpp:2632

[upsj]

@MarcelKoch thanks, I replaced them

[upsj]

@pratikvn Our types are first and foremost used by our code, and an array class that doesn't provide access to its values is at least inconvenient. Thrust has device_vector, RAPIDS has device_uvector, all of which provide this kind of access and more. There are many ways of getting poor performance out of a high performance library - we have the same issue with automatic cross-executor and cross-precision copies. I don't believe this is sufficient reason to avoid adding this to our fundamental storage type. To me, the amount of simplifications this allows speaks for itself.
If people do encounter poor performance, we have both the ProfilerHook and PerformanceHint loggers to help them find the source of their performance issues.

[pratikvn]

I still think the names load_value and store_value are misleading. In particular, because in my view load and store are verbs that are generally used for low level operations.

But I wont hold back the PR.

[upsj]

@pratikvn We have a kind of tendency in Ginkgo to give things elaborate names get_num_stored_elements, get_num_elems, copy_val_to_host, which I think we should cut back on. Arrays are a fundamental type, fundamental operations in fundamental types get short, concise (and established) names. This is also why I pushed against @thoasm suggestion of _to_host/_from_host suffixes.

I agree that load/store generally describe a lower abstraction level, but these functions should have concise names, and get/set_value wasn't considered clear enough.

[upsj]

In the latest state I added an accessor/device_reference-based interface, which has a slight readability advantage for writing values:

array.set_value(0, 1); // which is the index, which the value?
array.get_access()[0] = 0; // here it's clear

Still, these kinds of accessors bring all kinds of issues (e.g. the need to implement a lot of custom operators for cases where the matching operator is implemented with a template function, e.g. in std::complex)

[MarcelKoch]

Maybe going back one step and just implement

void set_element(array, index, value);
T get_element(array, index);

in a private header would be the better choice right now. From the discussion, this would be rather uncontroversial and already fixes the use cases we want to target here.

[upsj]

As the least intrusive version, I now moved to an internal access function

[pratikvn]

LGTM!

[sonarqubecloud]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud