Page MenuHomePhabricator

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

Authored by kiranktp on Jun 19 2020, 5:16 AM.

Details

Summary

This patch enhances parser support for flush construct to OpenMP 5.0 by including memory-order-clause.

2.17.8 flush Construct
!$omp flush [memory-order-clause] [(list)]

		where memory-order-clause is
		acq_rel
		release
		acquire

The patch includes code changes and testcase modifications.

Diff Detail

Event Timeline

kiranktp created this revision.Jun 19 2020, 5:16 AM
klausler accepted this revision.Jun 19 2020, 10:23 AM

LGTM; thanks!

This revision is now accepted and ready to land.Jun 19 2020, 10:23 AM

Hi All,

The commit for this ended up failing so I temporarily reverted it in 64b04e4754bfe7bf718e5140fe1fd0ca50373c28. Feel free to recommit on fix :)

Thanks!

-eric

This revision was automatically updated to reflect the committed changes.

Two comments.

memory-order-clause is defined for both atomic and flush instructions. We already have MemoryOrder
defined in the parse-tree for atomic in 4.5. In 4.5 memory-order can only be seq_cst. In 5.0 memory-order
for atomic can be seq_cst, acq_rel, release, acquire, relaxed. For flush it is only a subset and the allowed
values are only acq_rel, release, acquire.

Would it make sense to have a common definition for memory-order-clause and handle the fact that it
can only be a subset in semantics?

Flush is 2.17.8 (not 2.18.8) as part of the OpenMP 5.0 specification here. Is there a newer document?
https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf

Hi All,

The commit for this ended up failing so I temporarily reverted it in 64b04e4754bfe7bf718e5140fe1fd0ca50373c28. Feel free to recommit on fix :)

Thanks!

-eric

Hi Eric,
I had validated the patch with debug build and "ninja check-flang"/"ninja check-all" before pushing it for review on phabricator.
I didn't face any errors. Anyway, I will check with release build once.

Two comments.

memory-order-clause is defined for both atomic and flush instructions. We already have MemoryOrder
defined in the parse-tree for atomic in 4.5. In 4.5 memory-order can only be seq_cst. In 5.0 memory-order
for atomic can be seq_cst, acq_rel, release, acquire, relaxed. For flush it is only a subset and the allowed
values are only acq_rel, release, acquire.

Would it make sense to have a common definition for memory-order-clause and handle the fact that it
can only be a subset in semantics?

Flush is 2.17.8 (not 2.18.8) as part of the OpenMP 5.0 specification here. Is there a newer document?
https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf

Thanks for the review Kiran.

memory-order-clause is defined for both atomic and flush instructions. We already have MemoryOrder
defined in the parse-tree for atomic in 4.5. In 4.5 memory-order can only be seq_cst. In 5.0 memory-order
for atomic can be seq_cst, acq_rel, release, acquire, relaxed. For flush it is only a subset and the allowed
values are only acq_rel, release, acquire.

Would it make sense to have a common definition for memory-order-clause and handle the fact that it
can only be a subset in semantics?

I did thought of this approach, but keeping the definitions separate will be neat and will be easier to extend each construct for further changes in future.
I did see the same approach for couple of constructs handling.

Flush is 2.17.8 (not 2.18.8) as part of the OpenMP 5.0 specification here. Is there a newer document?
https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf

Thanks for pointing it out. I was referring to OpenMP 5.1 draft version as well. I have used the same section numbers.
I will change it back to OpenMP 5.0 section number.
Probably i will have to change it for other construct as well.

kiranktp updated this revision to Diff 272645.Jun 23 2020, 2:10 AM
kiranktp edited the summary of this revision. (Show Details)

Corrected the section number to 2.17.8 [from OpenMP 5.0] for flush construct

What is happening with this change? It looks like it was applied and then reverted.
@echristo, was there a problem?

What is happening with this change? It looks like it was applied and then reverted.
@echristo, was there a problem?

Hi @echristo,
I am not facing any test failures with this patch on my sandbox. I tested this on multiple setups.
Below are my build details:

env CC=clang-8 CXX=clang++-8 CXXFLAGS="-stdlib=libc++" LDFLAGS="-fuse-ld=lld" cmake -GNinja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX=../install_clang -DLLVM_TARGETS_TO_BUILD=host -DLLVM_INSTALL_UTILS=on -DLLVM_ENABLE_PROJECTS="mlir;flang" ../../llvm
ninja -j8 install
ninja check-all

Let me know if I am missing anything.

@echristo was seeing a failure when running the test "Semantics/omp-clause-validity01.f90" as per his revert commit message.

Tests pass for me. I built and tested on an ubuntu Aarch64 machine. Flang was built as a release with debug build using gcc-9.2 with ninja.

@echristo was seeing a failure when running the test "Semantics/omp-clause-validity01.f90" as per his revert commit message.

Tests pass for me. I built and tested on an ubuntu Aarch64 machine. Flang was built as a release with debug build using gcc-9.2 with ninja.

I'm happy to try again if you think that the current diff should work :)

-eric

@echristo was seeing a failure when running the test "Semantics/omp-clause-validity01.f90" as per his revert commit message.

Tests pass for me. I built and tested on an ubuntu Aarch64 machine. Flang was built as a release with debug build using gcc-9.2 with ninja.

Thank you for Validating Kiran!

@echristo was seeing a failure when running the test "Semantics/omp-clause-validity01.f90" as per his revert commit message.

Tests pass for me. I built and tested on an ubuntu Aarch64 machine. Flang was built as a release with debug build using gcc-9.2 with ninja.

I'm happy to try again if you think that the current diff should work :)

-eric

Hi Eric, I had shared my build details. Am I missing anything while building/testing?

Please do try and let me know if you still face the issue.