Add ISAI reference implementation

[thoasm]

This PR adds a reference implementation of ISAI (Incomplete Sparse Approximate Inverse).

Currently, I am not using a Composition internally because it is unable to handle multiple right hand sides. I will look into it and probably change that in the upcomming PR for the CUDA, HIP and OMP executor implementation.

[upsj]

LGTM in general, I have some comments on the class design and code itself though.

[thoasm]

All problems should now be resolved and most of the comments from @upsj are addressed.
We might have to discuss the responsibilities further.

[pratikvn]

Very nice extensive testing! Only some minor things.

[codecov]

Codecov Report

Merging #489 into develop will decrease coverage by 0.14%.
The diff coverage is 85.61%.

@@             Coverage Diff             @@
##           develop     #489      +/-   ##
===========================================
- Coverage    88.54%   88.40%   -0.15%     
===========================================
  Files          262      268       +6     
  Lines        16652    16923     +271     
===========================================
+ Hits         14745    14960     +215     
- Misses        1907     1963      +56     

Impacted Files	Coverage Δ
core/device_hooks/common_kernels.inc.cpp	0.00% <0.00%> (ø)
omp/matrix/csr_kernels.cpp	13.06% <ø> (ø)
omp/preconditioner/isai_kernels.cpp	0.00% <0.00%> (ø)
reference/matrix/csr_kernels.cpp	98.93% <ø> (ø)
core/preconditioner/isai.cpp	51.72% <51.72%> (ø)
include/ginkgo/core/preconditioner/isai.hpp	76.47% <76.47%> (ø)
core/test/preconditioner/isai.cpp	84.00% <84.00%> (ø)
reference/test/preconditioner/isai_kernels.cpp	94.20% <94.20%> (ø)
reference/preconditioner/isai_kernels.cpp	94.66% <94.66%> (ø)
include/ginkgo/core/base/lin_op.hpp	58.33% <0.00%> (-17.86%)	⬇️
... and 8 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c25c1e...57ee063. Read the comment docs.

[yhmtsai]

LGTM. only minor format changes

[tcojean]

Here: https://github.com/ginkgo-project/ginkgo/blob/develop/include/ginkgo/core/base/dim.hpp#L100

What would make this undefined behavior? Is that not covered by 5.7.5 in the standard? The only issue would be bound checking, but we have the assert.
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf

Edit: my bad, I now see what you mean maybe. first_ does not point to an array hence indeed that is rather an edge case here. This is supposing that rest_ comes directly after first_ and that the only thing in each of the recursive first_ elements are these values with no extra data or storage in between...

[upsj]

I think the relevant part here would be § 9.2.13:

Nonstatic data members of a (non-union) class with the same access control (Clause 11) are allocated so that later members have higher addresses within a class object. The order of allocation of non-static data members with different access control is unspecified (Clause 11). Implementation alignment requirements might cause two adjacent members not to be allocated immediately after each other;

[tcojean]

Yes indeed, with the code depending on first_ and rest_ (in fact each of the subsequent first_ in recursion) being laid out in a strict consecutive fashion, that is rather broken. Your section shows it clearly. I had completely forgotten this had been implemented like that...

[thoasm]

I also did not know or forgot that we store and access the dimensions that way. I assume that the intel compiler saw a chance to optimize there (to only copy the first_ part), which caused the second dimension to never be initialized (valgrind also noted that there was a conditional jump on an uninitialized variable).

[tcojean]

LGTM. Only some minor comments.

[thoasm]

@yhmtsai Unfortunately, moving the CXXFLAGS flags to the GINKGO_COMPILER_FLAGS did not work, as you can see in the output the github workflow here.
A reason might be that we do not use GINKGO_COMPILER_FLAGS when building the tests, but that is just a guess.
I will revert the change and leave it as was (with cflags == "-O1"), so it passes all workflows properly.

[hartwiganzt]

@thoasm ready to be merged?

[thoasm]

@hartwig Now that all tests succeeded, yes, I will merge it.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
No Duplication information