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

mpi: Generate deterministic code for overlap mode #2303

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

georgebisbas
Copy link
Contributor

@georgebisbas georgebisbas commented Feb 6, 2024

Overlap mode did not have deterministic code generation.

static void remainder0(struct dataobj *restrict u_vec, const float a, const float dt, float r0, float r1, float r2, int t0, int t1, const int x_M, const int x_m, const int y_M, const int y_m, const int nthreads)
{
  compute0(a,u_vec,dt,r0,r1,r2,t0,t1,x_M - 2,x_m + 2,MIN(y_M, y_m + 1),y_m,nthreads);
  compute0(a,u_vec,dt,r0,r1,r2,t0,t1,x_M - 2,x_m + 2,y_M,MAX(y_m, y_M - 1),nthreads);
  compute0(a,u_vec,dt,r0,r1,r2,t0,t1,MIN(x_M, x_m + 1),x_m,y_M - 2,y_m + 2,nthreads);
  compute0(a,u_vec,dt,r0,r1,r2,t0,t1,MIN(x_M, x_m + 1),x_m,MIN(y_M, y_m + 1),y_m,nthreads);
  compute0(a,u_vec,dt,r0,r1,r2,t0,t1,MIN(x_M, x_m + 1),x_m,y_M,MAX(y_m, y_M - 1),nthreads);
  compute0(a,u_vec,dt,r0,r1,r2,t0,t1,x_M,MAX(x_m, x_M - 1),y_M - 2,y_m + 2,nthreads);
  compute0(a,u_vec,dt,r0,r1,r2,t0,t1,x_M,MAX(x_m, x_M - 1),MIN(y_M, y_m + 1),y_m,nthreads);
  compute0(a,u_vec,dt,r0,r1,r2,t0,t1,x_M,MAX(x_m, x_M - 1),y_M,MAX(y_m, y_M - 1),nthreads);
}

compute0 calls could have random order.

Also, MIN/MAX macros could have random order. (this is a side-effect, not overlap specific)

To reproduce, you could run a script multiple times with DEVITO_MPI=overlap

@georgebisbas georgebisbas self-assigned this Feb 6, 2024
Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (313c980) 86.69% compared to head (a272ba3) 86.69%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2303   +/-   ##
=======================================
  Coverage   86.69%   86.69%           
=======================================
  Files         229      229           
  Lines       43043    43043           
  Branches     7983     7983           
=======================================
  Hits        37318    37318           
  Misses       5034     5034           
  Partials      691      691           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mloubout mloubout added the MPI mpi-related label Feb 7, 2024
devito/mpi/halo_scheme.py Outdated Show resolved Hide resolved
@@ -138,6 +138,8 @@ def relax_incr_dimensions(iet, options=None, **kwargs):
def generate_macros(iet):
applications = FindApplications().visit(iet)
headers = set().union(*[_generate_macros(i) for i in applications])
# Sort for deterministic code generation
headers = sorted(headers)
Copy link
Contributor

Choose a reason for hiding this comment

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

same filter_sorted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this I think filter-sorted is redundant. What would it offer more?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is conceptually wrong and useless:

Conceptually wrong because if all passes appending headers had to ensure uniquess it would be painful. BTW it's not even just the headers, it would be everything that can get passed to metadata.

Which brings us to the second point, useless: see here:
https://github.com/devitocodes/devito/blob/master/devito/passes/iet/engine.py#L123

Copy link
Contributor

Choose a reason for hiding this comment

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

obviously if you instead added it because it generates bad code, then there's a bug somewhere else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct! Thanks, it was a side-effect of the compute regions non-determinism, catching the first (min/max) area to be computed

@georgebisbas georgebisbas force-pushed the deterministic_overlap branch from 0c03f1a to bc63cc8 Compare February 7, 2024 12:22
@georgebisbas georgebisbas changed the title compiler: Generate deterministic code for overlap mode mpi: Generate deterministic code for overlap mode Feb 9, 2024
@georgebisbas
Copy link
Contributor Author

@mloubout @FabioLuporini this should be ok?

@@ -138,6 +138,8 @@ def relax_incr_dimensions(iet, options=None, **kwargs):
def generate_macros(iet):
applications = FindApplications().visit(iet)
headers = set().union(*[_generate_macros(i) for i in applications])
# Sort for deterministic code generation
headers = sorted(headers)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is conceptually wrong and useless:

Conceptually wrong because if all passes appending headers had to ensure uniquess it would be painful. BTW it's not even just the headers, it would be everything that can get passed to metadata.

Which brings us to the second point, useless: see here:
https://github.com/devitocodes/devito/blob/master/devito/passes/iet/engine.py#L123

@FabioLuporini FabioLuporini merged commit 14ade37 into master Feb 19, 2024
31 checks passed
@FabioLuporini
Copy link
Contributor

Thanks, merged

@FabioLuporini FabioLuporini deleted the deterministic_overlap branch February 19, 2024 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MPI mpi-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants