-
Notifications
You must be signed in to change notification settings - Fork 32
1623 add training #3145
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
base: master
Are you sure you want to change the base?
1623 add training #3145
Conversation
arporter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm enjoying going through (in outline) your game-of-life example - it's great to see that you can recover performance :-)
Since most of the example transformation scripts need their module and apply-method docstrings updating, I'm going to pause here to let you do that.
As you suggested, I'm basically ignoring the actual text of the readme's so I am by no means reviewing the actual tutorials.
tutorial/training/gocean/2.8-GameOfLife-openmp/fuse_loops_last.py
Outdated
Show resolved
Hide resolved
tutorial/training/gocean/2.4-GameOfLife-psyclone/solution/read_config_mod.f90
Show resolved
Hide resolved
|
Wow, that was an impressive review. I think it took me over an hour just to add 'done' to your comments. You rightly pointed out some repeated errors (generat function, missing docstrings, inlining not mentioned). I think I've fixed them up everywhere. I've triggered the full |
|
@arporter I have a problem - turns out the OpenMP offloading does not work (anymore??). Due to a bug in the makefile, it actually hasn't applied the offloading script at all in my recent tests (I am pretty certain it did originally, but I did some refactoring/ simplifications of the Makefile, which I believe introduced this bug recently). Now I've fixed the Makefile, it aborts with: (see https://github.com/stfc/PSyclone-mirror/actions/runs/20805411529/job/59758702708) Googling indicates that there might be a mapping missing (which we don't support afaik), or additional compiler options(?). I don't know what to do. I don't have (easy) access to hardware that would support managed memory, and I am not even certain what the right solution would be in this case :( Can you (or Sergi?) help? I assume you can log onto the machine and try running it there? |
…ran would compile openmp offloading, but that doesn't work then).
|
As discussed, I have added a I only triggered the compilation IT (to speed things up), since I expect there will be another round of changes, and no code here should affect anything else outside of training. |
arporter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it all the way through! I like your various examples - they are impressive.
Mostly it's just a bit more tidying of docstrings.
I have to admit that I'm not that keen on the use of the .x90 extension on files that are just standard .f90. This could cause confusion with the the Met Office approach where only files written using the DSL have the .x90 extension. I can see that it perhaps makes it easier to automatically run PSyclone on the necessary files but you could just name them in the Makefile.
| # Enable OpenMP on CPU (for some run tests) | ||
| F90=mpif90 F90FLAGS="-mp -Minfo=all" make -C tutorial/training test_run | ||
| USE_GPU=no F90=mpif90 F90FLAGS="-mp -Minfo=all" make -C tutorial/training test_run | ||
| # Delete any previous foles, and compile the openacc example with openacc: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"files"
| The trainings material is split into four parts, and the files | ||
| and documentation here are only the hands-on section. The full | ||
| training is accompanied by a presentations | ||
| (a slightly outdated version of the slides are available on the `wiki |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I thought the slides were separate material to the training - I'd forgotten you created them. I like your suggestion since the training READMEs etc. are available in the repo anyway.
tutorial/training/gocean/2.16-GameOfLife-omp-offload/solution/output_field_mod.f90
Outdated
Show resolved
Hide resolved
| # Create the PSyIR, and get the variable access information: | ||
| reader = FortranReader() | ||
| psyir = reader.psyir_from_source(code) | ||
| routine = psyir.find_routine_psyir("foo") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't seem to use routine but perhaps you should s/psyir.children[0]/routine/ below?
| print("digraph {") | ||
|
|
||
| # Handle each variable | ||
| for var in varinfo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also nice. Made me remember that I want to move Habbakuk (https://github.com/arporter/habakkuk) over to PSyIR and this is pretty much it.
|
|
||
| do j=ystart, ystop | ||
| do i=xstart, xstop | ||
| current(i, j) = current(i, j) - die(i, j) + born(i, j) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a .f90?
|
|
||
| subroutine combine(current, die, born) | ||
|
|
||
| implicit none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.f90?
| # arguments passed to the first invocation of Make. The last entry | ||
| # in this list is the current file. | ||
| this_file := $(abspath $(lastword $(MAKEFILE_LIST))) | ||
| # PSyclone directory is up two from this file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
three?
Adds the PSyclone training material (based on #3131)