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

Issue #249. OCLTrans() generates OpenCL kernels (also fixes fparser2 stmt_fns) #387

Merged
merged 30 commits into from
Jun 12, 2019

Conversation

sergisiso
Copy link
Collaborator

This PR (based on issue #249 ) enables the automatic generation of OpenCL kernels referenced by the FortCL PSy layer. This works for the examples/gocean/eg3 and NemoLite2D (both needed a temporal fix for the array assignments wrongly parsed as statement functions by fparser2)

@codecov-io
Copy link

codecov-io commented May 21, 2019

Codecov Report

Merging #387 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #387      +/-   ##
==========================================
+ Coverage   97.55%   97.56%   +<.01%     
==========================================
  Files          24       24              
  Lines       10245    10273      +28     
==========================================
+ Hits         9995    10023      +28     
  Misses        250      250
Impacted Files Coverage Δ
src/psyclone/psyGen.py 95.38% <100%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4035127...752e2f0. Read the comment docs.

@sergisiso sergisiso self-assigned this Jun 3, 2019
@sergisiso sergisiso requested a review from arporter June 6, 2019 14:45
@sergisiso
Copy link
Collaborator Author

@arporter This PR is ready for review. This is a set of small changes for the OCLTrans() transformation to automatically generate the kernels associated with OpenCL PSy-layer. These are:

  • rename_and_write now looks for opencl marked kernels and assign .cl names.
  • Statement Functions are corrected to assignments when possible.
  • updated examples/gocean/eg3 to use GO_ prefix and updated README.
  • OpenCL back-end now always use 'e' for scientific notation (instead of Fortran 'd').
  • Unit test produces temporary files in a tmpdir directory.
  • Updated documentation.

@sergisiso
Copy link
Collaborator Author

@arporter Argh, sorry, 1 more commit to bring this up to master. But it is ready if it passes the tests.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

All looks good.
Documentation has been updated and all three guides build OK.
I have some minor comments - see inline.
Main issues are that I get 5 test failures when running the test suite in parallel with compilation testing enabled (--compile and --compileopencl). Also, with Rupert's imminent change to fparser, it will no longer produce Statement_Function objects in the Parse Tree so one test will need updating.
I'll check pylint next time.

examples/gocean/eg3/README Outdated Show resolved Hide resolved
examples/gocean/eg3/README Outdated Show resolved Hide resolved
examples/gocean/eg3/compute_z_mod.f90 Outdated Show resolved Hide resolved
examples/gocean/eg3/compute_z_mod.f90 Outdated Show resolved Hide resolved
src/psyclone/psyGen.py Show resolved Hide resolved
src/psyclone/psyGen.py Outdated Show resolved Hide resolved
src/psyclone/psyGen.py Outdated Show resolved Hide resolved
src/psyclone/psyGen.py Outdated Show resolved Hide resolved
src/psyclone/tests/psyGen_test.py Outdated Show resolved Hide resolved
src/psyclone/tests/psyGen_test.py Show resolved Hide resolved
src/psyclone/psyGen.py Show resolved Hide resolved
src/psyclone/psyGen.py Show resolved Hide resolved
src/psyclone/tests/psyGen_test.py Outdated Show resolved Hide resolved
Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Only minor things left now plus a need to update the documentation to be a bit clearer about what happens when creating OpenCL kernels.
Once those things are done, this is ready to go.

src/psyclone/tests/gocean1p0_opencl_test.py Outdated Show resolved Hide resolved
src/psyclone/tests/psyGen_test.py Show resolved Hide resolved
src/psyclone/tests/psyGen_test.py Show resolved Hide resolved
src/psyclone/tests/psyclone_test_utils.py Show resolved Hide resolved
src/psyclone/psyGen.py Show resolved Hide resolved
src/psyclone/tests/gocean1p0_opencl_test.py Outdated Show resolved Hide resolved
src/psyclone/tests/psyGen_test.py Outdated Show resolved Hide resolved
src/psyclone/tests/psyGen_test.py Outdated Show resolved Hide resolved
@sergisiso
Copy link
Collaborator Author

@arporter Ready for next review

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Almost, almost there. I was going to accept it until I discovered that we now have an XPASSing test (a test marked as xfail that now passes). This is test_module_inline_and_compile in gocean1p0_transformations_test.py. Given how hard this was to find I suggest we change the default behaviour of pytest so that an XPASS is reported as a failure. We can do this by adding:

# Ensure that any XPASS ("unexpectedly passing") results are reported
# as failures in the test suite.
[tool:pytest]
xfail_strict=true

to the PSyclone/setup.cfg file. Would you mind doing this here?

There are also a few tiny things that need fixing - see inline.

doc/user_guide/transformations.rst Outdated Show resolved Hide resolved
src/psyclone/psyGen.py Outdated Show resolved Hide resolved
src/psyclone/psyGen.py Outdated Show resolved Hide resolved
@sergisiso
Copy link
Collaborator Author

@arporter Ready for the next review, I made the changes we discussed this morning.

@arporter
Copy link
Member

All requested changes have been made.
All tests pass or xfail, including with compilation.
One additional test has been marked as xfail because @arporter will address this in #315.

@arporter arporter merged commit c4dd939 into master Jun 12, 2019
@arporter arporter deleted the 249_OCLTransKernels branch June 12, 2019 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants