Adding reference counting to ParCSRMatrix

[rfalgout]

• We already have reference counting for the Struct and SStruct matrices, and this adds it to ParCSRMatrix.
• With some changes in src/krylov, reference counting would allow users to destroy the preconditioning matrix passed in through SetPrecondMatrix(). Add this later, and require the following instead.
• Require (for now) that users not destroy the matrix passed in through SetPrecondMatrix() until the solver is no longer needed and destroyed.

[rfalgout]

Hi @oseikuffuor1 . I made a logic change to pcg.c that occurs to me should be enough to fix the issues people are having. We would just require (for now) that when SetPrecondMatrix() is called, that matrix should not be destroyed prematurely. We can add the reference counting later.

Would you please test out the mixed precision PCG cases to make sure this is working correctly? I have already run some of our standard regression tests and this looks good in the single precision case. If this looks okay, I will go ahead and make similar changes to the other Krylov solvers.

Thanks!

[oseikuffuor1]

Hi @oseikuffuor1 . I made a logic change to pcg.c that occurs to me should be enough to fix the issues people are having. We would just require (for now) that when SetPrecondMatrix() is called, that matrix should not be destroyed prematurely. We can add the reference counting later.

Would you please test out the mixed precision PCG cases to make sure this is working correctly? I have already run some of our standard regression tests and this looks good in the single precision case. If this looks okay, I will go ahead and make similar changes to the other Krylov solvers.

Thanks!

@rfalgout I just tested the mixed-precision PCG on the branch and it all looks good.

Thanks.

[rfalgout]

Hi @oseikuffuor1 and @liruipeng . There were some difficult-to-find memory leaks that I had to fix - ParCSRMatrixCreate is not the only routine that creates matrices, so I needed to set the reference counter there also. I have changed the handling of precond_Mat in the other two Krylov routines that use it, and I did some prototype and casting clean up as well. I'm running regression tests now, but this is ready for you to review. Thanks!

[rfalgout]

Hi @oseikuffuor1 and @liruipeng . Just following up on this PR. It's ready for final review. Thanks!

[oseikuffuor1]

Should this comment be above the preceding line or did you intend to remove it?

[rfalgout]

I just moved the comment to the end of the precond_Mat declaration line. I thought it looked better. :)

[oseikuffuor1]

Looks good the me, thank you!

[liruipeng]

Hi @oseikuffuor1 and @liruipeng . There were some difficult-to-find memory leaks that I had to fix - ParCSRMatrixCreate is not the only routine that creates matrices, so I needed to set the reference counter there also. I have changed the handling of precond_Mat in the other two Krylov routines that use it, and I did some prototype and casting clean up as well. I'm running regression tests now, but this is ready for you to review. Thanks!

Hi @rfalgout . I guess when a parcsr matrix is not constructed using the constructor, we have to manually set the ref counter, such as in several places you did

matrix = hypre_CTAlloc(hypre_ParCSRMatrix, 1, HYPRE_MEMORY_HOST);
hypre_ParCSRMatrixRefCount(matrix) = 1;

which makes sense. Just curious if we have covered all of these cases.
Thanks

[liruipeng]

Looks good to me. Thanks @rfalgout

[rfalgout]

Hi @rfalgout . I guess when a parcsr matrix is not constructed using the constructor, we have to manually set the ref counter, such as in several places you did

matrix = hypre_CTAlloc(hypre_ParCSRMatrix, 1, HYPRE_MEMORY_HOST);
hypre_ParCSRMatrixRefCount(matrix) = 1;

which makes sense. Just curious if we have covered all of these cases. Thanks

I am pretty sure I covered all the cases. I did an appropriately-constructed grep for allocation of the matrix structure, and didn't find anything other than what's been fixed here. Thanks!

[nmnobre]

Do we think this could grant a minor 3.0.1 release?

[rfalgout]

Do we think this could grant a minor 3.0.1 release?

Yes, I think we can do that. Stay tuned.