Page MenuHomePhabricator

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

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

Details

Summary

Name resolution was probably failing in bug reported at
https://bugs.llvm.org/show_bug.cgi?id=48145
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

directive.

  • 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

TimeTest
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
flang/lib/Semantics/resolve-directives.cpp
368–369

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

sameeranjoshi added inline comments.Dec 16 2020, 10:24 AM
flang/lib/Semantics/resolve-directives.cpp
368–369

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
flang/lib/Semantics/resolve-directives.cpp
368–369

Would be better to have a single function.

Merge ResolveName into ResolveOmpName.

tskeith added inline comments.Dec 18 2020, 10:24 AM
flang/lib/Semantics/resolve-directives.cpp
1065

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.
flang/lib/Semantics/resolve-directives.cpp
1065

Thanks.
Not sure what I was trying to do here.

clementval added inline comments.Dec 18 2020, 11:23 AM
flang/lib/Semantics/resolve-directives.cpp
1000

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.

1074

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?

1076

Are those tabs?

sameeranjoshi marked 3 inline comments as done.Jan 4 2021, 1:32 AM
sameeranjoshi added inline comments.
flang/lib/Semantics/resolve-directives.cpp
1000

No it doesn't will remove the redundant one.

1074

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
  return

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?

1076

no.

kiranchandramohan requested changes to this revision.Jan 19 2021, 3:52 PM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-directive-structure.h
278–282

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

flang/lib/Semantics/check-omp-structure.cpp
243
260

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

flang/lib/Semantics/resolve-directives.cpp
334

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

1003

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