Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/hisq fgi #1368

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Feature/hisq fgi #1368

wants to merge 5 commits into from

Conversation

cpviolator
Copy link
Member

This WIP add functionality to allow for offload in the FGI RHMC routines in https://github.com/callat-qcd/milc_qcd/tree/feature/nested-force-gradient-integrator. It simply creates copy gauge and momentum fields consistent with MILC's QUDA offload sequences.

@cpviolator cpviolator requested a review from a team as a code owner March 17, 2023 19:42
@@ -124,6 +124,7 @@ void printQudaGaugeParam(QudaGaugeParam *param) {
P(use_resident_mom, 0);
P(make_resident_gauge, 0);
P(make_resident_mom, 0);
P(use_fgi, 1);// FGI: This needs to be 0, and set to 1 with MILC/QUDA interface
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be set back to 0, I'm guessing?

@@ -104,6 +103,32 @@ CloverField *cloverEigensolver = nullptr;
cudaGaugeField *momResident = nullptr;
cudaGaugeField *extendedGaugeResident = nullptr;

// These Copy fields are to support the Force Gradient Integrator
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if I like the term "Copy"... maybe "Backup" would be a better term? "Previous"? I guess that would be best informed by the language of the FGI algorithm --- and if "copy" is the common language, that's good enough to me. (This also isn't a hill I'll die on!)

@@ -2086,8 +2142,11 @@ quda::cudaGaugeField *checkGauge(QudaInvertParam *param)
param->cuda_prec_refinement_sloppy, param->cuda_prec_eigensolver};
QudaReconstructType recon[4] = {gaugeSloppy->Reconstruct(), gaugePrecondition->Reconstruct(),
gaugeRefinement->Reconstruct(), gaugeEigensolver->Reconstruct()};

printfQuda("QudaInvertParam has different precisions than loadGaugeQuda, so recomputing sloppy fields\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the addition of this for logging reasons---I'd put this in a logQuda(QUDA_VERBOSE, [...]);, I think.

@@ -2120,8 +2179,10 @@ quda::cudaGaugeField *checkGauge(QudaInvertParam *param)
// recon is always no for fat links, so just use long reconstructs here
QudaReconstructType recon[4] = {gaugeLongSloppy->Reconstruct(), gaugeLongPrecondition->Reconstruct(),
gaugeLongRefinement->Reconstruct(), gaugeLongEigensolver->Reconstruct()};
printfQuda("QudaInvertParam has different precisions than loadGaugeQuda, so recomputing sloppy fields\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above


// Wilson Links
//----------------------------------------------------------
if(gaugePrecise) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirming --- it's intentional that this doesn't throw an errorQuda if gaugePrecise is unset, right? I see that it's fully safe code on its own.


// Wilson Links
//----------------------------------------------------------
if(gaugePrecise) gaugePrecise->copy(*gaugeCopyPrecise);
Copy link
Contributor

Choose a reason for hiding this comment

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

if (gaugePrecise && gaugeCopyPrecise)

@@ -1381,6 +1406,37 @@ void endQuda(void)

if(momResident) delete momResident;

// Delete MILC FGI fields
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we discussed moving these into freeUniqueGauge, etc, offline --- just adding this comment so it can be marked as "resolved" at some point.

@@ -87,6 +87,8 @@ extern "C" {
int return_result_gauge; /**< Return the result gauge field */
int return_result_mom; /**< Return the result momentum field */

int use_fgi; /**< add FGI terms */
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this should be a bool instead? (Note that I'm going to kill QudaBoolean at some point, and replace it with bool - this is fine since bool is supported on C99 which we require anyway for the QUDA C interface.)

@@ -1373,6 +1375,18 @@ extern "C" {
*/
void momResidentQuda(void *mom, QudaGaugeParam *param);

/**
Copy link
Member

Choose a reason for hiding this comment

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

Some punctuation in this comment might be nice :P

@maddyscientist
Copy link
Member

maddyscientist commented Mar 29, 2023

Nothing controversial in this commit. One thought on the design used here: would it be a better fit to push and pop gauge / momentum fields. E.g., instead of backing up the resident fields and zeroing them, and the later restoring them, we just push a new field (which pushes the resident field onto the proverbial stack) and when we're done with this temporary, we simply pop it, freeing the temporary and restoring the prior resident field off of the stack.

Just a thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants