Skip to content

Mixed-precision parallel linear solver#7048

Open
nrseman wants to merge 19 commits into
OPM:masterfrom
haugenlabs:mixed-istl
Open

Mixed-precision parallel linear solver#7048
nrseman wants to merge 19 commits into
OPM:masterfrom
haugenlabs:mixed-istl

Conversation

@nrseman

@nrseman nrseman commented May 12, 2026

Copy link
Copy Markdown

This PR leverages OPM's ISTL framework to provide a parallel implementation of the mixed-precision linear solver. For more information refer to opm/simulators/linalg/mixed/README.md

@GitPaean GitPaean added the manual:irrelevant This PR is a minor fix and should not appear in the manual label May 12, 2026
@atgeirr atgeirr added manual:new-feature This is a new feature and should be described in the manual and removed manual:irrelevant This PR is a minor fix and should not appear in the manual labels May 13, 2026

@SoilRos SoilRos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it looks good, I just left some minor concerns. I still need to make the performance tests though.

Comment thread opm/simulators/linalg/mixed/bsr.h
Comment thread opm/simulators/linalg/mixed/bsr.c Outdated
Comment thread opm/simulators/linalg/mixed/SolverAdapter.hpp Outdated
Comment thread opm/simulators/linalg/mixed/MatrixWrapper.hpp Outdated
@SoilRos

SoilRos commented Jun 2, 2026

Copy link
Copy Markdown
Member

The changes look good. To merge, please resolve the merge conflicts, if possible, by rebasing the branch onto the current master.

@bska

bska commented Jun 2, 2026

Copy link
Copy Markdown
Member

To merge, please resolve the merge conflicts, if possible, by rebasing the branch onto the current master.

When you do, however, please replace the binary literals (0b10...10, 0b01...01) with hex constants (0xAA for 0b10...10 and 0x55 for 0b01...01) instead. While supported in C++ since C++14, binary literals in C requires C23 mode which we (OPM) currently don't use. The GCC compiler supports binary literals in earlier C versions as an extension, but other compilers do not and we've already had to address this once–see PR #7057. We would prefer to not do it again.

@nrseman

nrseman commented Jun 2, 2026

Copy link
Copy Markdown
Author

I just rebased the PR branch, addressed the merge conflicts, and replaced binary literals with their hex counterparts.

@SoilRos

SoilRos commented Jun 3, 2026

Copy link
Copy Markdown
Member

jenkins build this please

}

// downcast to single precision
bsr_downcast(M_);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update of the matrix transposes the blocks of the original matrix into the new buffer, in double precision. Later it downcasts the entire matrix to its single precision buffer. Now, I don't understand why to have the double precision in the first place? After all, the operations in this wrapper only operate on the single precision matrix. Maybe I am missing something here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your observation is correct, @SoilRos! This is an artifact from the original mixed-precision implementation where the interface was at the solver level. In that case, the double-precision matrix was required for incomplete factorization purposes. Now that the interfaces are at the preconditioner and linear operator levels, the original design has introduced some redundancies. The situation is actually slightly worse for the preconditioner. Addressing the issue is not that difficult, and will be taken care of. Of course, using column-major blocks throughout OPM would eliminate almost all redundancies ;-)

@SoilRos

SoilRos commented Jun 11, 2026

Copy link
Copy Markdown
Member

I have spent some more time with this code and I have gathered other observations. These are mostly about making this code more generic to make it fit a bit more with the modularity of the solvers.

  1. The matrix adapter is templated with the vector type and its block length. I think a better template parameter would be to mimic the matrix: block type and an allocator. The
    vector type can be a template parameter of the vector operations.
  2. I’d suggest to use a BCRSMatrix (or ideally IBCRSMatrix from Add support for experimental alternative of Dune::BCRSMatrix #7124) as the matrix type. In this form, the mixed operations can be trivially implemented for other block lengths and architectures. In the case of IBCRSMatrix, the pattern can be shared with the original matrix.
  3. In this PR, the optimized scalar product seems to be specific to the mixed adapter, but this holds for any general scalar product. IMO, if this is that general, it should replace the general case. Furthermore, these optimization seem to be very similar to the ones implemented by the standard library algorithm std::transform_reduce for the unordered execution policy (both in libstdc++ and libc++), so I suggest to try this algorithm. If performance is similar, use it instead as it is much more portable.
  4. The matrix operator is exchanged via a template specialization, it only takes into account overlapping Schwartz operator and matrix adapter. However, OPM has more implementations of matrix operators. We need to either implement the other adapters, or fail if these are instantiated. Silently changing the operator doesn't seem an option.
  5. The inverse operator for the mixed solver is only for BiCGSTABSolver, exchanging the underlying matrix operator. I suggest to change the name from MixedAdapter to MixedBiCGSTABSolver.

I already have partially implemented 1, 2, and 5.

@nrseman

nrseman commented Jun 22, 2026

Copy link
Copy Markdown
Author

@SoilRos: Can you clarify which of the above items are required to approve this PR for merging. I think 1,4, and 5, are relatively straight forward, but 2 and 3 introduces scope creep and I suggest holding off on those for now. Any objections?

@SoilRos

SoilRos commented Jun 25, 2026

Copy link
Copy Markdown
Member

Yes, I think 1, 4, and 5 are fine for the scope of this PR.

@nrseman

nrseman commented Jul 2, 2026

Copy link
Copy Markdown
Author

@SoilRos, I just pushed three more commits to address the items discussed. If you are happy, I'll rebase on the current master so that we can get this merged and move on to the next one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual:new-feature This is a new feature and should be described in the manual

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants