Page MenuHomePhabricator

[Flang][openmp] Add semantic checks for OpenMP critical construct.
Needs RevisionPublic

Authored by sameeranjoshi on Dec 10 2020, 9:35 AM.



Name resolution was probably failing in bug reported at
as the omp critical construct needs a new symbol for the name which was
missing. This is fixed by creating a new symbol during name resolution
for a new flag OmpCriticalLock.

Upgrade to OMP-5.0 standard.

Checks for restrictions from OMP-5.0 are added.

  • If a name is specified on a critical directive, the same name must also be specified on the

end critical directive.

  • If no name appears on the critical directive, no name can appear on the end critical


  • Unless the effect is as if hint(omp_sync_hint_none) was specified, the critical

construct must specify a name.

A new routine CheckNameMatching mostly follows CheckEndName from resolve-labels.cpp.

Diff Detail

Unit TestsFailed

80 msx64 windows > LLVM.CodeGen/XCore::threads.ll
Script: -- : 'RUN: at line 1'; c:\ws\w32-1\llvm-project\premerge-checks\build\bin\llc.exe -march=xcore < C:\ws\w32-1\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll | c:\ws\w32-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w32-1\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll

Event Timeline

sameeranjoshi created this revision.Dec 10 2020, 9:35 AM
sameeranjoshi requested review of this revision.Dec 10 2020, 9:35 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript
clementval added inline comments.Dec 16 2020, 8:41 AM

What's the big difference with ResolveOmpName. Merging the two functions would be better.

sameeranjoshi added inline comments.Dec 16 2020, 10:24 AM

That's a good catch.
If you see ResolveName was implemented before this patch.
This patch highlighted the need to extend the name resolution logic, which was just extended inside ResolveName and no other code was affected(except that we need a one time signature change with this patch).
As discussed, there is no way I could find to reuse code from resolve-names.cpp inside this file, hence we had a short/quick way to implement ResolveName inside this file.
Being said that I still see with some new construct the ResolveName inside this file might need some change.
Hence I extracted into another function.
Which makes only changes to the above function and not much changes in other code.

If there's still something you see of no use to have separate functions I can refactor them and collapse them into 1.
Thank you.

clementval added inline comments.Dec 16 2020, 10:54 AM

Would be better to have a single function.

Merge ResolveName into ResolveOmpName.

tskeith added inline comments.Dec 18 2020, 10:24 AM

What is the purpose of introducing maybeName when you can just use name?

Remove unnecessary variable mayBeName.

sameeranjoshi marked 3 inline comments as done.Dec 18 2020, 10:57 AM
sameeranjoshi added inline comments.

Not sure what I was trying to do here.

clementval added inline comments.Dec 18 2020, 11:23 AM

Does it really make sense to push two contexts? If yes, you might want to have a visitor function for parser::OmpCriticalDirective and parser::OmpEndCriticalDirective where you push their respective context.


Do you want the program to abort or would it be better to have a return value to this function and generate appropriate user facing error message?


Are those tabs?

sameeranjoshi marked 3 inline comments as done.Jan 4 2021, 1:32 AM
sameeranjoshi added inline comments.

No it doesn't will remove the redundant one.


I think aborting it seems better.
Consider a test case which uses Symbol::Flags other than ompFlagsRequireNewSymbol.
for e.g Using shared clause and there is a missing declaration for symbol foo.

integer function timer_tick_sec()
  implicit none
  integer t

  !$OMP parallel shared(foo)
  t = t + 1
  !$OMP END parallel

  timer_tick_sec = t

end function timer_tick_sec

Above test case gives the user error message as
No explicit type declared for 'foo' which seems the correct error for above test case as it's pointing the errors in users code.

DIE seems to be an error internal to the compiler and there could be a problem with the ResolveOmpName and the user might not be responsible for that.

If you have a test which you think might fail, could you please point it?



kiranchandramohan requested changes to this revision.Jan 19 2021, 3:52 PM
kiranchandramohan added inline comments.

Can you use parser::Unwrap<parser::Name> to simplify the code?


Can this be part of Enter? Or is there a convention that checking clauses will only happen during leave?


Does lock need a new symbol? What is the issue you saw?


Can the name here be a variable declared elsewhere? If not do we have to resolve the name?

This revision now requires changes to proceed.Jan 19 2021, 3:52 PM