Added OpenMP 5.0 specification based semantic checks for CRITICAL con…

[NimishMishra]

…struct name resolution

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

[NimishMishra]

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

All of the parser changes have already been done in https://github.com/NimishMishra/f18-llvm-project/commit/7a22c267bfbc910cfa3fbffcbee2e093ed8950c0 , so those are skipped

[schweitzpgi]

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)};
                                                                 ^

[kiranchandramohan]

I cannot compile either.

[NimishMishra]

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

[NimishMishra]

@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]

@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]

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

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

[NimishMishra]

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

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

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

[jeanPerier]

Hi, I have just rebased fir-dev on LLVM 85bf221f204eafc1142a064f1650ffa9d9e03dad 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]

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

[NimishMishra]

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]

Gentle ping!

[NimishMishra]

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

Sure. I'll upstream it

[kiranchandramohan]

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?