[flang-commits] [PATCH] D93051: [Flang][openmp] Add semantic checks for OpenMP critical construct.

[sameeran joshi via Phabricator via flang-commits]

sameeranjoshi marked 3 inline comments as done.
sameeranjoshi added inline comments.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1000
+  PushContext(beginCriticalDir.source, llvm::omp::Directive::OMPD_critical);
+  PushContext(endCriticalDir.source, llvm::omp::Directive::OMPD_critical);
+  if (const auto &criticalName{
----------------
clementval wrote:
> 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. 
No it doesn't will remove the redundant one.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1074
+    } else {
+      DIE("OpenMP Name resolution failed");
+    }
----------------
clementval wrote:
> 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? 
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?


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1076
+    }
+  }
+
----------------
clementval wrote:
> Are those tabs? 
no.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93051/new/

https://reviews.llvm.org/D93051