Belos CGSingleReduce: merge allreduces #4302
Conversation
…ce detection This change allows using the norm induced by the preconditioner for convergence detection. Since CG already computed this, we save on all-reduce. On the flip-side, specifying tolerance wrt the preconditioner norm might be more difficult.
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Using Repos:
Pull Request Author: cgcgcg |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
There was a problem hiding this comment.
Thanks for working on this! A couple issues:
-
Is there any reason you would ever not want this optimization? I don't even think it should be an option. Just always do this, if it makes sense (e.g., if
useSingleReduction_istrue). -
There's no test that turns this option on. Since it's off by default, it will never get tested. (Fixing (1) will ensure this feature always gets tested, without you needing to do any work :-) .)
| @@ -516,6 +521,9 @@ setParameters (const Teuchos::RCP<Teuchos::ParameterList> ¶ms) | |||
| // Check if the user is requesting the single-reduction version of CG (only for blocksize == 1) | |||
| if (params->isParameter("Use Single Reduction")) { | |||
| useSingleReduction_ = params->get("Use Single Reduction", useSingleReduction_default_); | |||
| if (useSingleReduction_) | |||
There was a problem hiding this comment.
What happens if users don't enable "Use Single Reduction", but enable the new option? Does it just silently ignore the request?
There was a problem hiding this comment.
Yes, that's correct.
There was a problem hiding this comment.
Could it ever be possible to fold the convergence test into the all-reduce, if "Use Single Reduction" is true? If not, then Belos should report an error in this case.
| @@ -370,6 +370,8 @@ namespace Belos { | |||
| static constexpr int verbosity_default_ = Belos::Errors; | |||
| static constexpr int outputStyle_default_ = Belos::General; | |||
| static constexpr int outputFreq_default_ = -1; | |||
| static constexpr const char * resNorm_default_ = "TwoNorm"; | |||
| static constexpr bool foldConvergenceDetectionIntoAllreduce_default_ = false; | |||
There was a problem hiding this comment.
Is there any reason why this should not default to true? Users won't know about this option and so they will always get the default. Could this ever not be a good idea? If it's always a good idea, then you should always do it. It should not even be an option. Just do the faster thing.
There was a problem hiding this comment.
I did not change the default norm that is used for convergence detection, i.e. the 2-norm. Switching this to true saves one allreduce, but incurs more local computation. My guess is that this only pays off once the allreduce is expensive enough, so not for small communicator size.
There was a problem hiding this comment.
@cgcgcg Do you have a sense for the trade-off point? Most people won't use single-reduce CG unless they know what it is and care about the cost of all-reduces.
| if (foldConvergenceDetectionIntoAllreduce_) { | ||
| T_ = MVT::Clone( *tmp, 2 ); | ||
| // Z_ will view the first column of T_, R2_ will view the second. | ||
| std::vector<int> index(1,0); |
There was a problem hiding this comment.
You could also use the overloads of CloneViewNonConst that take a Teuchos::Range1D.
|
Also, @iyamazaki wrote a Tpetra-specific implementation of single-reduce CG. You can get it from |
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
cgcgcg
requested review from
mhoemmen and
hkthorn
and removed request for
hkthorn
cgcgcg
force-pushed
the
belosMergeAllReduces
branch
from
3e8fc4f to
f9b0665
Compare
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Using Repos:
Pull Request Author: cgcgcg |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
@cgcgcg I am fine with the changes. I need for there to be a test added to ensure that this modification to the solver works and continues working. |
cgcgcg
added
the
AT: WIP
This adds the option to combine the all-reduces for CG's inner products and the convergence detection into one for BelosCGSingleRedIter. The trade-off is more computation.
cgcgcg
force-pushed
the
belosMergeAllReduces
branch
from
f9b0665 to
6116417
Compare
cgcgcg
removed
the
AT: WIP
cgcgcg
force-pushed
the
belosMergeAllReduces
branch
from
6116417 to
7f25bb2
Compare
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Using Repos:
Pull Request Author: cgcgcg |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
1 similar comment
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
| @@ -370,6 +370,8 @@ namespace Belos { | |||
| static constexpr int verbosity_default_ = Belos::Errors; | |||
| static constexpr int outputStyle_default_ = Belos::General; | |||
| static constexpr int outputFreq_default_ = -1; | |||
| static constexpr const char * resNorm_default_ = "TwoNorm"; | |||
| static constexpr bool foldConvergenceDetectionIntoAllreduce_default_ = false; | |||
There was a problem hiding this comment.
@cgcgcg Do you have a sense for the trade-off point? Most people won't use single-reduce CG unless they know what it is and care about the cost of all-reduces.
| @@ -516,6 +521,9 @@ setParameters (const Teuchos::RCP<Teuchos::ParameterList> ¶ms) | |||
| // Check if the user is requesting the single-reduction version of CG (only for blocksize == 1) | |||
| if (params->isParameter("Use Single Reduction")) { | |||
| useSingleReduction_ = params->get("Use Single Reduction", useSingleReduction_default_); | |||
| if (useSingleReduction_) | |||
There was a problem hiding this comment.
Could it ever be possible to fold the convergence test into the all-reduce, if "Use Single Reduction" is true? If not, then Belos should report an error in this case.
| // Get the norms of the residuals native to the solver. | ||
| template <class ScalarType, class MV, class OP> | ||
| Teuchos::RCP<const MV> | ||
| CGSingleRedIter<ScalarType,MV,OP>::getNativeResiduals( std::vector<MagnitudeType> *norms ) const { |
There was a problem hiding this comment.
It's technically possible to call this method with norms == nullptr, though I suppose the "iter" classes are just implementation details of the "SolMgr" classes.
| // | ||
| MVT::MvAddMv( one, *cur_soln_vec, alpha, *P_, *cur_soln_vec ); | ||
| lp_->updateSolution(); | ||
| if (foldConvergenceDetectionIntoAllreduce_) { |
There was a problem hiding this comment.
Not a huge fan of the massive code duplication here.
There was a problem hiding this comment.
I can certainly pull the if-statement into the loop. I think it's easier to read that way, since there are a couple of changes between the two paths..
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
2 similar comments
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
@mhoemmen @hkthorn I ran a comparison using the 2-norm for convergence detection, and two test problems that are in the MiniEM app: a mass matrix with diagonal preconditioning, and a second order differential operator with MG preconditioner. The break-even point appeared to be around 4000 ranks. One reason why the new code path is slow is that we require one extra preconditioner apply per solve and overconverge. (In principle this is not terrible. since users could change the tolerance to account for that.) |
|
@cgcgcg Thanks for the information, it is very useful to know. There are pluses and minuses to every variation of the communication reducing variants. |
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ hkthorn ]! |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
jwillenbring
added
the
AT: RETEST
|
I applied the retest flag to this PR because we added a new PR test this afternoon that enables CUDA. Given the status of this PR, I hope this will not negatively impact your effort. |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
Using Repos:
Pull Request Author: cgcgcg |
mhoemmen
dismissed
their stale review
Heidi says OK; Christian explained his reasoning. Thanks!
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3_SERIAL
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_9.2
Jenkins Parameters
|
trilinos-autotester
removed
the
AT: RETEST
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
@trilinos/belos
Description
The CG single reduce iteration performs two allreduces per iteration: one for the inner products and one for the convergence detection.
The proposed changes allow to merge these into a single allreduce per iteration. The options for that are:
The default behavior has not been changed.