Skip to content

Initial support for OpenACC syntax #224

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

Merged
merged 10 commits into from
Jun 30, 2021
Merged

Conversation

jefflarkin
Copy link

@jefflarkin jefflarkin commented Apr 14, 2021

This PR creates initial support for OpenACC directives, based on the existing OpenMP syntax.

Changes:
Made OpenMP sentinel less greedy
Added initial support for OpenACC directives

Future work:

  • I would love to implement the actual grammar so that clauses that are invalid on particular directives do not match. Currently this does the same simple matching as the OpenMP syntax on which I based this change.
  • Does not currently match the arguments to data clauses, may be a nice addition, but they are a bit complex to match
  • Does not currently match optional clause modifiers, like readonly: and zero:

Jeff Larkin added 3 commits April 13, 2021 20:49
Made OpenMP sentinel less greedy
Added initial support for OpenACC directives

WIP:
Some clauses not matching properly.
Directives with optional arguments may not match properly.
@jefflarkin jefflarkin changed the title WIP - Initial support for OpenACC syntax Initial support for OpenACC syntax Apr 14, 2021
@jefflarkin jefflarkin marked this pull request as ready for review April 14, 2021 20:20
@jefflarkin jefflarkin mentioned this pull request Apr 14, 2021
@gnikit
Copy link
Member

gnikit commented Apr 17, 2021

The one thing that would be useful would be to add a Fortran file where you test these directives. I added some myself under test/resources I found it extremely helpful while making changes to visually inspect that I have not broken anything and it will be useful in the future when we figure out how we can unittest the syntax highlighting.

Also, as I said in my tracking issue, could you think of any cases where the OpenMP functionality would fail with the change that you made?

@jefflarkin
Copy link
Author

Sure. I will add the test cases I was using when I'm back at that computer on Monday.

Regarding the OpenMP matching, what I found was that with the existing matching it would sometimes erroneously match an OpenACC directive with the OpenMP syntax. For instance 'acc parallel do' is not valid, but it would get matched from the OpenMP. I made the matching less greedy and as best as I could tell still match correctly.

Adds resource file with extensive OpenACC directives to check.
Multi-line matching appears to not work as expected in OpenACC or OpenMP
@jefflarkin
Copy link
Author

jefflarkin commented Apr 19, 2021

@gnikit I added resources/test/openacc_support.f90. I'm working through multi-line matching at the moment. This doesn't appear to work as expected in either the OpenMP or OpenACC. If either & is removed, it matches properly, but if both are there it doesn't match.

I included a test at the bottom to test that the OpenMP is not over-matching the OpenACC, but is still correctly matching on its own.

@jefflarkin
Copy link
Author

I submitted #225 related to line continuations, but since this is true for the existing OpenMP and OpenACC, I don't think it should block the merging of this issue. The fix should be the same for each.

@jefflarkin
Copy link
Author

Fixes #225

@jefflarkin
Copy link
Author

@gnikit I believe this is ready to merge and put out in the market. Do you agree?

Copy link
Member

@gnikit gnikit left a comment

Choose a reason for hiding this comment

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

Awesome job @jefflarkin. Thanks for taking the time to do this.
I had a quick play around with my OpenMP and the added OpenACC test and all looks good.

It truly is a shame that we cannot unify the openmp.json and openacc.json into one file since they have a lot of duplicate code, but I think it is better to add support now for the openacc and improve on it in the future, if need be.

I left some minor comments, but no major changes.

I look forward to seeing this in master.

@gnikit gnikit self-assigned this Jun 30, 2021
@gnikit gnikit linked an issue Jun 30, 2021 that may be closed by this pull request
@gnikit gnikit linked an issue Jun 30, 2021 that may be closed by this pull request
@gnikit
Copy link
Member

gnikit commented Jun 30, 2021

I don't know if @krvajal you want to have a look. All is good. I will add the updated CHANGELOG in a bit.

@jefflarkin
Copy link
Author

Fixes #184

@gnikit gnikit linked an issue Jun 30, 2021 that may be closed by this pull request
@gnikit gnikit merged commit 6da12de into fortran-lang:master Jun 30, 2021
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.

OpenMP continuation not triggering correctly OpenACC highlighting
2 participants