Page MenuHomePhabricator

[Flang][OpenMP][OpenACC] Fix exit out of a region in OpenMP parallel construct.
ClosedPublic

Authored by sameeranjoshi on Oct 1 2020, 7:27 AM.

Details

Summary

From below mentioned standard references
OpenACC 3.0 Standards document
840 • A program may not branch into or out of an OpenACC parallel construct

OpenMP 5.0 Standards document
A program that branches into or out of a parallel region is non-conforming.

This patch
Resolves the issue of exit out of a parallel region, other branching out issues like goto statements are not handled with this patch.
Moves code from D87906 to be reused by other OpenMP/OpenACC to check-directive-structure.h.
Adds support in OpenMP parallel construct and a test case to verify.

Diff Detail

Event Timeline

sameeranjoshi created this revision.Oct 1 2020, 7:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2020, 7:27 AM
sameeranjoshi requested review of this revision.Oct 1 2020, 7:27 AM
sameeranjoshi edited subscribers, added: tskeith; removed: yaxunl, guansong, sstefan1.

Rebased on master and updated after D87906.

clementval requested changes to this revision.Oct 19 2020, 11:54 AM
clementval added inline comments.
flang/lib/Semantics/check-acc-structure.cpp
89

DirectiveStructureChecker:: is not necessary here and it's not added for other checks.

96

DirectiveStructureChecker:: is not necessary here and it's not added for other checks.

This revision now requires changes to proceed.Oct 19 2020, 11:54 AM

Remove unnecessary DirectiveStructureChecker:: from check-acc-structure.cpp and check-omp-structure.cpp.

clementval added inline comments.Oct 21 2020, 11:37 AM
flang/test/Semantics/omp-clause-validity01.f90
169

Is the indentation of directive on purpose?

sameeranjoshi added inline comments.Oct 21 2020, 11:38 AM
flang/test/Semantics/omp-clause-validity01.f90
169

I was trying to use a tool to make formatting like clang-format named fprettify.
I will see how other code is.

Fix indentation.

clementval accepted this revision.Oct 27 2020, 8:48 AM

LGTM just a nit comment if you can update before landing this patch.

flang/lib/Semantics/check-directive-structure.h
29

nit: Blank line before this block of code.

This revision is now accepted and ready to land.Oct 27 2020, 8:48 AM

Nit: I believe this only handles exit out of a region. It does not fully handle the branching (goto etc) out issue. Can you update the title and description to reflect this?

sameeranjoshi retitled this revision from [Flang][OpenMP][OpenACC] Support branching out issue in OpenMP parallel construct. to [Flang][OpenMP][OpenACC] Fix exit out of a region in OpenMP parallel construct..Oct 27 2020, 10:46 AM
sameeranjoshi edited the summary of this revision. (Show Details)
sameeranjoshi added a project: Restricted Project.

Sadly this patch is causing build failures on our Flang buildbots: http://lab.llvm.org:8011/#/builders/33/builds/386

FAILED: tools/flang/lib/Semantics/CMakeFiles/obj.FortranSemantics.dir/check-acc-structure.cpp.o 
/usr/bin/clang++-10  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/flang/lib/Semantics -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/include -Itools/flang/include -Iinclude -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/include -isystem /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/../mlir/include -isystem tools/mlir/include -isystem tools/clang/include -isystem /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/../clang/include -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Werror -Wno-deprecated-copy -Wno-string-conversion -Wno-unused-command-line-argument -Wstring-conversion           -Wcovered-switch-default -Wno-nested-anon-types -O3     -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/flang/lib/Semantics/CMakeFiles/obj.FortranSemantics.dir/check-acc-structure.cpp.o -MF tools/flang/lib/Semantics/CMakeFiles/obj.FortranSemantics.dir/check-acc-structure.cpp.o.d -o tools/flang/lib/Semantics/CMakeFiles/obj.FortranSemantics.dir/check-acc-structure.cpp.o -c /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics/check-acc-structure.cpp
In file included from /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics/check-acc-structure.cpp:8:
In file included from /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics/check-acc-structure.h:17:
/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics/check-directive-structure.h:39:9: error: field 'currentDirective_' will be initialized after field 'upperCaseDirName_' [-Werror,-Wreorder-ctor]
        currentDirective_{directive}, upperCaseDirName_{
        ^
/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics/check-directive-structure.h:273:25: note: in instantiation of member function 'Fortran::semantics::NoBranchingEnforce<llvm::acc::Directive>::NoBranchingEnforce' requested here
  NoBranchingEnforce<D> noBranchingEnforce{
                        ^
/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/lib/Semantics/check-acc-structure.cpp:89:5: note: in instantiation of member function 'Fortran::semantics::DirectiveStructureChecker<llvm::acc::Directive, llvm::acc::Clause, Fortran::parser::AccClause, 45>::CheckNoBranching' requested here
    CheckNoBranching(block, GetContext().directive, blockDir.source);
    ^

I've also noticed other patches causing build failures. Fixing this one felt rather straightforward so I took the liberty of submitting a fix: https://github.com/llvm/llvm-project/commit/b7d1271a01b72a079ea016a9d4e31a73091d5f0e. I hope that that's fine.

I've also noticed other patches causing build failures. Fixing this one felt rather straightforward so I took the liberty of submitting a fix: https://github.com/llvm/llvm-project/commit/b7d1271a01b72a079ea016a9d4e31a73091d5f0e. I hope that that's fine.

Thanks for fixing this @awarzynski

I've also noticed other patches causing build failures. Fixing this one felt rather straightforward so I took the liberty of submitting a fix: https://github.com/llvm/llvm-project/commit/b7d1271a01b72a079ea016a9d4e31a73091d5f0e. I hope that that's fine.

Thanks a lot @awarzynski I would add to my local builds -DFLANG_ENABLE_WERROR=ON from now onwards.