Added OpenMP 5.0 specification based semantic checks for CRITICAL con… #1128

NimishMishra · 2021-10-13T13:13:21Z

…struct name resolution

Cherry picking https://reviews.llvm.org/D110502 onto fir-dev branch

NimishMishra · 2021-10-13T13:25:13Z

@kiranchandramohan @clementval Could you please see if this is fine?

All of the parser changes have already been done in NimishMishra@7a22c26 , so those are skipped

flang/test/Semantics/omp-sync-critical01.f90

flang/test/Semantics/omp-sync-critical02.f90

schweitzpgi · 2021-10-13T17:19:11Z

I get compilation failures with this patch.

check-omp-structure.cpp:1020:65: error: ?const struct Fortran::parser::Verbatim? has no member named ?t?
   const auto &dirName{std::get<std::optional<parser::Name>>(dir.t)};
                                                                 ^

schweitzpgi

Does not compile.

I assume there is something that needs to be chery-picked from main here.

kiranchandramohan · 2021-10-14T11:17:51Z

I cannot compile either.

NimishMishra · 2021-10-15T03:45:49Z

Fixed build failures. @kiranchandramohan @schweitzpgi it is compiling for me now. Please check once.

kiranchandramohan

I still have a build failure. See inline comment.

Also, have two failed tests after fixing the build failure.
Failed Tests (2):
Flang :: Lower/OpenMP/critical.f90
Flang :: Semantics/omp-sync-critical02.f90

flang/lib/Semantics/resolve-directives.cpp

NimishMishra · 2021-10-17T16:28:21Z

@kiranchandramohan Please check this PR. For me, ninja check-flang has one test case failing. But I am not sure how this relates to my PR. Perhaps this has something to do with Lowering that the fir-dev branch has. Any ideas on what changes in my PR could be causing this? I am not at all versed with lowering yet

FAIL: Flang :: Lower/OpenMP/critical.f90 (8 of 1157)
******************** TEST 'Flang :: Lower/OpenMP/critical.f90' FAILED ********************
Script:
--
: 'RUN: at line 3';   bbc -fopenmp -emit-fir /home/manwe/Desktop/f18-llvm-project/flang/test/Lower/OpenMP/critical.f90 -o - |    /home/manwe/Desktop/f18-llvm-project/build/bin/FileCheck /home/manwe/Desktop/f18-llvm-project/flang/test/Lower/OpenMP/critical.f90 --check-prefix=FIRDialect
: 'RUN: at line 5';   bbc -fopenmp /home/manwe/Desktop/f18-llvm-project/flang/test/Lower/OpenMP/critical.f90 -o - |    tco --disable-llvm --print-ir-after=fir-to-llvm-ir 2>&1 |    /home/manwe/Desktop/f18-llvm-project/build/bin/FileCheck /home/manwe/Desktop/f18-llvm-project/flang/test/Lower/OpenMP/critical.f90 --check-prefix=LLVMIRDialect
: 'RUN: at line 8';   bbc -fopenmp -emit-fir /home/manwe/Desktop/f18-llvm-project/flang/test/Lower/OpenMP/critical.f90 -o - |    tco | /home/manwe/Desktop/f18-llvm-project/build/bin/FileCheck /home/manwe/Desktop/f18-llvm-project/flang/test/Lower/OpenMP/critical.f90 --check-prefix=LLVMIR
--
Exit Code: 2

Command Output (stderr):
--
bbc: /home/manwe/Desktop/f18-llvm-project/flang/lib/Lower/PFTBuilder.cpp:1315: int {anonymous}::SymbolDependenceDepth::analyze(const Fortran::semantics::Symbol&): Assertion `symTy && "symbol must have a type"' failed.

kiranchandramohan · 2021-10-17T21:53:20Z

flang/lib/Semantics/resolve-directives.cpp

    const auto pair{GetContext().scope.try_emplace(
-        name.source, Attrs{}, UnknownDetails{})};
+        name.source, Attrs{}, ObjectEntityDetails{})};


If you set it back to UnknownDetails it will work. I think for ObjectEntityDetails there are additional requirements.

Thanks. This solved the erring test case.

Will @clementval @jeanPerier or @schweitzpgi know the importance of using the correct details here? AFAIU, for the name of critical sections we do not need ObjectEntityDetails.

@klausler is it OK to use UnknownDetails here? This is for the name of an OpenMP Critical Section for which I think the ObjectEntityDetails do not apply.

UnknownDetails is mostly a place-holder intended to be replaced once more is known about a symbol.

@NimishMishra can you test @klausler's point, i.e what happens when we have a critical construct whose name matches a variable in the same scope?

The OpenMP standard says the following,
The names of critical constructs are global entities of the program. If a name conflicts with any other entity, the behavior of the program is unspecified.

The following works fine in fir-dev branch:

program sample use omp_lib integer :: foo, bar bar = 0 !$omp critical (foo) bar = bar + 1 !$omp end critical (foo) end program sample

Running as flang-new -fc1 -fopenmp <FILENAME>.f90 gives no semantic problems at all.

Could you also check conflicts with construct names (like a named do loop ) ?

I checked. The semantics phase doesn't complain.

Great thanks ! Could you add a regression test (I guess a symbol test would be the best (like these tests)) with both the variable name and construct name that are the same as the critical name just to be sure this property continues to hold true.

flang/lib/Semantics/resolve-directives.cpp

kiranchandramohan · 2021-10-20T14:11:46Z

@sscalpone @schweitzpgi : Can you add @NimishMishra to this repo so that he can add reviewers and request reviewers? @NimishMishra works for Flang at AMD.

kiranchandramohan · 2021-10-27T10:03:05Z

I think we should not have merge commits in the PRs. Can you make this PR as a few commits,

The cherry-picked upstream commit + any minor changes required to apply the patch.
Changes for MiscDetails.

NimishMishra · 2021-10-27T12:20:39Z

I think we should not have merge commits in the PRs. Can you make this PR as a few commits,

The cherry-picked upstream commit + any minor changes required to apply the patch.

Changes for MiscDetails.

@kiranchandramohan I have collapsed all into one commit. Please have a look

flang/lib/Semantics/resolve-directives.cpp

jeanPerier · 2021-10-28T13:16:32Z

Hi, I have just rebased fir-dev on LLVM 85bf221 from last week.
That means you will need to update this PR with the rebased fir-dev.
You cannot simply rebase your PR with fir-dev (it will try to reapply all fir-dev changes).

You can update the PR as follow (assuming your branch is called my-branch):

backup your branch into my-branch-bckup (just to be safe :) )
create a new branch my-branch-new following the new fir-dev head
cherry-pick all your commits from this PR into my-branch-new
once it looks good, force push my-branch-new into the upstream my-branch (git push origin my-branch-new:my-branch -f )

Sorry for the inconvenience, I could not wait for every PR to get in and solve all new conflicts on my side, the rebase
is meant to help upstreaming so that we can avoid having this too many times again in the future.

kiranchandramohan · 2021-11-22T12:07:09Z

Can you add the two tests that were suggested?
Also, change the title and description of the PR to match the present contents.

NimishMishra · 2021-12-13T13:41:37Z

Can you add the two tests that were suggested? Also, change the title and description of the PR to match the present contents.

If I run flang-new -fc1 -fopenmp normally, things are fine. But if I do flang-new -fc1 -fdebug-unparse-with-symbols -fopenmp, flang-new crashes. But this happens only when critical construct name and some other program entity's name (an integer for example) collides (check the XFAIL test case).

NimishMishra · 2022-01-24T08:15:10Z

Gentle ping!

kiranchandramohan

LGTM.
I guess this has to be now added to upstream llvm-project as well.

NimishMishra · 2022-04-01T04:06:03Z

LGTM. I guess this has to be now added to upstream llvm-project as well.

Sure. I'll upstream it

kiranchandramohan · 2022-04-13T14:39:08Z

Can you add the two tests that were suggested? Also, change the title and description of the PR to match the present contents.

If I run flang-new -fc1 -fopenmp normally, things are fine. But if I do flang-new -fc1 -fdebug-unparse-with-symbols -fopenmp, flang-new crashes. But this happens only when critical construct name and some other program entity's name (an integer for example) collides (check the XFAIL test case).

I think it should not crash. But not sure whether it is an issue with unparsing. Does -fdebug-dump-symbols pass?

clementval reviewed Oct 13, 2021

View reviewed changes

flang/test/Semantics/omp-sync-critical01.f90 Outdated Show resolved Hide resolved

clementval reviewed Oct 13, 2021

View reviewed changes

flang/test/Semantics/omp-sync-critical02.f90 Outdated Show resolved Hide resolved

NimishMishra force-pushed the fir-dev branch from 4dc1d32 to 237e49b Compare October 13, 2021 13:36

schweitzpgi requested review from jeanPerier and kiranchandramohan October 13, 2021 17:14

schweitzpgi requested changes Oct 13, 2021

View reviewed changes

kiranchandramohan reviewed Oct 15, 2021

View reviewed changes

flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved

kiranchandramohan reviewed Oct 17, 2021

View reviewed changes

NimishMishra force-pushed the fir-dev branch from 7a6948e to cc0da2f Compare October 27, 2021 12:19

kiranchandramohan reviewed Oct 27, 2021

View reviewed changes

flang/lib/Semantics/resolve-directives.cpp Show resolved Hide resolved

jeanPerier force-pushed the fir-dev branch from f72c975 to d680db3 Compare October 28, 2021 13:04

NimishMishra force-pushed the fir-dev branch 2 times, most recently from 780cea1 to b07c821 Compare November 13, 2021 09:31

NimishMishra force-pushed the fir-dev branch 2 times, most recently from cc0da2f to 04a4c55 Compare December 13, 2021 13:40

NimishMishra added 2 commits December 13, 2021 22:00

Relaced UnknownDetails with MiscDetails

7008421

Added an XFAIL test case for critical construct name collision

04a4c55

schweitzpgi self-requested a review January 25, 2022 20:01

kiranchandramohan approved these changes Apr 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added OpenMP 5.0 specification based semantic checks for CRITICAL con… #1128

Added OpenMP 5.0 specification based semantic checks for CRITICAL con… #1128

NimishMishra commented Oct 13, 2021

NimishMishra commented Oct 13, 2021 •

edited

Loading

schweitzpgi commented Oct 13, 2021

schweitzpgi left a comment

kiranchandramohan commented Oct 14, 2021

NimishMishra commented Oct 15, 2021

kiranchandramohan left a comment

NimishMishra commented Oct 17, 2021

kiranchandramohan Oct 17, 2021

NimishMishra Oct 18, 2021

kiranchandramohan Oct 18, 2021

kiranchandramohan Oct 20, 2021

klausler Oct 20, 2021

kiranchandramohan Oct 27, 2021

NimishMishra Oct 28, 2021

jeanPerier Oct 28, 2021

NimishMishra Oct 28, 2021

jeanPerier Oct 28, 2021

kiranchandramohan commented Oct 20, 2021

kiranchandramohan commented Oct 27, 2021

NimishMishra commented Oct 27, 2021

jeanPerier commented Oct 28, 2021

kiranchandramohan commented Nov 22, 2021

NimishMishra commented Dec 13, 2021

NimishMishra commented Jan 24, 2022

kiranchandramohan left a comment

NimishMishra commented Apr 1, 2022

kiranchandramohan commented Apr 13, 2022

Added OpenMP 5.0 specification based semantic checks for CRITICAL con… #1128

Are you sure you want to change the base?

Added OpenMP 5.0 specification based semantic checks for CRITICAL con… #1128

Conversation

NimishMishra commented Oct 13, 2021

NimishMishra commented Oct 13, 2021 • edited Loading

schweitzpgi commented Oct 13, 2021

schweitzpgi left a comment

Choose a reason for hiding this comment

kiranchandramohan commented Oct 14, 2021

NimishMishra commented Oct 15, 2021

kiranchandramohan left a comment

Choose a reason for hiding this comment

NimishMishra commented Oct 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiranchandramohan commented Oct 20, 2021

kiranchandramohan commented Oct 27, 2021

NimishMishra commented Oct 27, 2021

jeanPerier commented Oct 28, 2021

kiranchandramohan commented Nov 22, 2021

NimishMishra commented Dec 13, 2021

NimishMishra commented Jan 24, 2022

kiranchandramohan left a comment

Choose a reason for hiding this comment

NimishMishra commented Apr 1, 2022

kiranchandramohan commented Apr 13, 2022

NimishMishra commented Oct 13, 2021 •

edited

Loading