This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Enhance parser support for atomic construct to OpenMP 5.0
ClosedPublic

Authored by kiranktp on Jun 30 2020, 10:16 PM.

Details

Summary

This patch enhances parser support for atomic construct to OpenMP 5.0.

2.17.7 atomic -> ATOMIC [clause [,]] atomic-clause [[,] clause] |

          ATOMIC [clause]
clause -> memory-order-clause | HINT(hint-expression)
memory-order-clause -> SEQ_CST | ACQ_REL | RELEASE | ACQUIRE | RELAXED
atomic-clause -> READ | WRITE | UPDATE | CAPTURE

The patch includes code changes and testcase modifications.

Diff Detail

Event Timeline

kiranktp created this revision.Jun 30 2020, 10:16 PM
DavidTruby accepted this revision.Jul 6 2020, 2:55 AM

Looks good to me! As a nit, perhaps you could add some tests that shouldn't parse correctly as well?

This revision is now accepted and ready to land.Jul 6 2020, 2:55 AM

Looks good to me! As a nit, perhaps you could add some tests that shouldn't parse correctly as well?

Thanks for reviewing the code David!.

As a nit, perhaps you could add some tests that shouldn't parse correctly as well?

Any syntax error will lead to chain of errors. It will be tough to add a case for invalid syntax.
I will check this once.

Any syntax error will lead to chain of errors. It will be tough to add a case for invalid syntax.

Please consider adding error recovery to prevent the cascade of errors. See the docs and the parser for examples.

Any syntax error will lead to chain of errors. It will be tough to add a case for invalid syntax.

Please consider adding error recovery to prevent the cascade of errors. See the docs and the parser for examples.

Yes Steve. Checking this.

kiranktp updated this revision to Diff 281524.Jul 29 2020, 5:31 AM

Used Common openmp Directives and added a negative test case.

Looks OK to me.

Two questions inline. Please add a few more tests covering all new memory-orders.

flang/include/flang/Parser/parse-tree.h
3631

Is the source field used by the parser? I didn't see a sourced parser for OmpAtomicMemoryOrderClause.

flang/test/Semantics/omp-atomic.f90
14

Can you use the omp_lib module and add tests for hints with the enum/kind value? like HINT(OMP_LOCK_HINT_CONTENDED)

kiranktp added inline comments.Sep 1 2020, 12:21 AM
flang/include/flang/Parser/parse-tree.h
3631

I will remove the source field as it is not used.

flang/test/Semantics/omp-atomic.f90
14

Sure. i will use omp_lib module and replace constants with Synchronization Hints.

kiranktp updated this revision to Diff 289090.Sep 1 2020, 12:29 AM

Incorporated review comments.

kiranktp marked 2 inline comments as done.Sep 1 2020, 12:30 AM
sameeranjoshi accepted this revision.Sep 7 2020, 10:52 AM
This revision was landed with ongoing or failed builds.Sep 7 2020, 6:22 PM
This revision was automatically updated to reflect the committed changes.