Add csr mixed spmv and accessor storage addr

[yhmtsai]

It adds the csr mixed spmv without casting the vectors to the matrix value type.

Additionally, I add the get_storage_address(index) in the accessor because I need the original pointer to perform Atomic operation.
Note: It might be possible to have the atomicCAS to still perform arithmetic type on the atomic operation.

[thoasm]

I would prefer if you changed the approach to the atomic_add with accessors.

[thoasm]

I am not a big fan of adding this operation and using it inside other kernels.
There is already an accessor (that is not pushed to develop yet) that operates on the POSIT format which would not work with this at all.
Preferably, the accessor either gets a atomic_add member function, or we have a free-standing function that takes the accessor and the index to perform the atomic operation (which could leverage this get_storage_address for reduced_row_major).

[yhmtsai]

Could we put the compute_index into the public interface? It's currently in the protected interface. I can simply compute the storage address out of the accessor without touching atomic add.
Will POSIT have its own math operator? (like half) or it can only be used as accessor storage.
if it is like half and we only use the specific length like 32bit, 64 bits, atomic_add still works for it by passing through atomic_cas.
If it is the storage type of accessor, it will not work with the current atomic_add. Need to use some overload to call atomicCAS

[thoasm]

The POSIT is simply a storage format, so it can only be converted to and from IEEE types and all arithmetic operations happen in IEEE.
Making compute_index public would be fine with me, however, relying on that inside the CSR kernel defeats the purpose of having an Accessor interface since it only works with reduced_row_major.
That is the reason why I would prefer to have another function that handles support for more than one accessor. Inside that function, it would be fine to rely on get_storage_address or compute_index.

I need to redesign the whole accessor structure to make this more clear and easier to use, which I will do after finishing the block-compressor accessor.

[upsj]

This discussion only seems relevant if we plan to add POSIT support (or other, more advanced storage types) as a ValueType in Ginkgo in the near future, and with non-existent hardware support, I don't really see that happening yet.

Honestly, this kind of function to me is a hint that the accessor is not a suitable abstraction in this case. Considering that it is mainly a wrapper around a static_cast on load/store, I'm not convinced the complexity is worth it.

[thoasm]

Not as a ValueType, but as a storage format. You get the same problem when you want to add bfloat16 and other storage formats to it, which I would like to have in the future.

[thoasm]

Not sure what scaling issues you mean @upsj . The 2 implementations I was referring to was the accessor and the CSR SpMV.

[upsj]

I meant the comment "but that doesn't scale well"

[thoasm]

Oh, that "scaling" was referring to having multiple implementation that do the same thing, which leads to bad maintainability.
I didn't mean performance scaling, sorry for the confusion.

[upsj]

I was also talking about programming complexity. The kernel either uses accessors or it doesn't, IMO there is no need to maintain two implementations.

[thoasm]

Since this is likely not the only place where we need some atomic access, I would prefer of solving it with the accessor once and use that in more places.
But if you think it is better to implement it without the accessor, feel free to do so.

[thoasm]

LGTM!
Most of my comments encourage the creation of helper functions to create an accessor out of a dense matrix.

[thoasm]

Can this comment be removed / moved as you don't have the atomic add in this place anymore?

[yhmtsai]

I will prefer putting it here because casting to the output storage type is here

[thoasm]

Notes about the introduced helper functions.

[thoasm]

I would prefer if you move this file into core/matrix/csr_helper or similar as this is a helper to transition from Accessor to Ginkgo (and is dependent on Ginkgo).

[thoasm]

As a helper, this should indicate one way or the other what accessor is used underneath.
This could be accomplished by putting this into a struct with the concrete accessor as the template parameter (allows for specialization for more accessors), a name change (to indicate the accessor like build_rrm_accessor or build_reduced_row_major_accessor) or by putting this into the CSR namespace and only using it there.

[upsj]

Considering how many surprising auto deductions happen in the code - are we taking the right approach with the accessors here? A more straightforward implementation would need to consist of only two things: 1. additional template parameters and 2. a static_cast around every load from/store to vals/input/output vector.

With the decaying of reference-like types happening only with the first operation with another object, this reminds me a lot of the issues that std::vector<bool> and Eigen's expression templates have. The C++ standard library maintainers need to take great care that all algorithms work correctly on iterators returning reference-like types, but I'm not sure we should be forced to do the same? Unfortunately, there is no operator auto() that automatically decays these transient objects on assignment.

[yhmtsai]

are we taking the right approach with the accessors here? A more straightforward implementation would need to consist of only two things: 1. additional template parameters and 2. a static_cast around every load from/store to vals/input/output vector.

I thought it is the target of the accessor. Use operation on a different format than storage.
if no accessor, I will also use the static_cast for these operations. although it will be lengthy, it's simple and clear.

[upsj]

I thought it is the target of the accessor. Use operation on a different format than storage.

The accessor is a tool for abstracting away loads and stores, which is well-suited for regular BLAS1/2 kernels, but with the Csr SpMV being our most performance-critical component, and the memory accesses being the bottleneck of the operation, I think it is important to have fine control over the memory operations. Things we might want to try out (if the compiler doesn't figure out to do it for us already) involve nontemporal or L1-bypassing stores, noncoherent loads, prefetching etc, and for all that I think is easiest if we have the memory ops behind at most a thin abstraction.

[yhmtsai]

rebase!