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: Instrument compute0 core after specialising as ComputeCall #2143

Merged
merged 3 commits into from
Jun 28, 2023

Conversation

georgebisbas
Copy link
Contributor

@georgebisbas georgebisbas commented Jun 9, 2023

This PR aims to make compute0 "more special" for instrumenting

and then adds a timer for the compute0 core area in FULL mpi mode (for advanced2 profiling)

e.g now we get:

Global performance: [OI=6.55, 15.50 GFlops/s, 0.19 GPts/s]
Local performance:
  * section0[rank0]<21,50,50,8,8,40> ran in 0.72 s [OI=6.55, 15.56 GFlops/s, 0.19 GPts/s]
    + haloupdate0 ran in 0.00 s [0.01%]
    + halowait0 ran in 0.00 s [0.01%]
    + remainder0 ran in 0.55 s [76.68%]
    + compute0 ran in 0.17 s [23.32%]

Generated code-diff:

*** 42,48 ****
    double haloupdate0;
    double halowait0;
    double remainder0;
-   double compute0;
  } ;
  
  struct region0
--- 42,47 ----
***************
*** 81,89 ****
      START_TIMER(haloupdate0)
      haloupdate0(u_vec,comm,msg0,6,t0,nthreads);
      STOP_TIMER(haloupdate0,timers)
-     START_TIMER(compute0)
      compute0(a,msg0,u_vec,dt,r0,r1,r2,r3,t0,t1,x0_blk0_size,x_M - 16,x_m + 16,y0_blk0_size,y_M - 16,y_m + 16,z_M - 16,z_m + 16,nthreads);
-     STOP_TIMER(compute0,timers)
      START_TIMER(halowait0)
      halowait0(u_vec,t0,msg0,6,nthreads);
      STOP_TIMER(halowait0,timers)
--- 80,86 ----

@georgebisbas georgebisbas added the MPI mpi-related label Jun 9, 2023
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #2143 (cf1ba20) into master (772243b) will increase coverage by 0.02%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##           master    #2143      +/-   ##
==========================================
+ Coverage   87.09%   87.11%   +0.02%     
==========================================
  Files         223      223              
  Lines       39871    39886      +15     
  Branches     5170     5170              
==========================================
+ Hits        34726    34748      +22     
+ Misses       4568     4562       -6     
+ Partials      577      576       -1     
Impacted Files Coverage Δ
devito/ir/iet/efunc.py 78.08% <95.23%> (+0.30%) ⬆️
devito/mpi/routines.py 93.22% <100.00%> (+0.03%) ⬆️
devito/operator/profiling.py 69.71% <100.00%> (+0.94%) ⬆️
devito/passes/iet/instrument.py 80.00% <100.00%> (+7.27%) ⬆️
tests/test_mpi.py 98.86% <100.00%> (+<0.01%) ⬆️

@georgebisbas georgebisbas self-assigned this Jun 9, 2023
@georgebisbas georgebisbas changed the title mpi: Specialise compute0 call and function mpi: Instrument compute0 core after specialising as ComputeCall Jun 9, 2023
@georgebisbas georgebisbas force-pushed the compute0_special branch 2 times, most recently from c5508ed to 921e8aa Compare June 16, 2023 14:14
@@ -1025,6 +1025,23 @@ def __init__(self, name, body, parameters):
super(HaloUpdate, self).__init__(name, body, parameters)


class ComputeFunction(ElementalFunction):
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't scale -- if you had 12 different kinds of Callables, what would you do? 12 different routines, one for each of them?

Here's an alternative

In ir/iet/efuncs.py, alter:

class ElementalFunction(...):

    _Call_cls = ElementalCall

    ...

    def make_call(...):
          return self._Call_cls(...)

Also, the fact that your make_compute_func is essentially a copy-paste of make_efunc should have smelled bad

You can extend make_efunc s.t.

def make_efunc(..., efunc_type=None):
    ...

so eventually here in routines.py all you will need to do is:

class ComputeCall(ElementalCall):
    pass

class ComputeFunction(ElementalFunction):
    _Call_cls = ComputeCall

Then calling make_efunc(..., efunc_type=ComputeFunction)

I'm not claiming this is exceptionally clean (the whole make_efunc thing seems like an overkill to me, but what can we do, it's been around for ages...), but at least it scales

@georgebisbas
Copy link
Contributor Author

Requested changes have ben addressed

Copy link
Contributor

@FabioLuporini FabioLuporini left a comment

Choose a reason for hiding this comment

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

This LGTG to me now. @mloubout ?

@mloubout mloubout merged commit 3d94984 into master Jun 28, 2023
@mloubout mloubout deleted the compute0_special branch June 28, 2023 18:10
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