[PATCH] D110714: [Flang][openmp] Added semantic checks for atomic construct with update clause

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan edited reviewers, added: klausler; removed: ftynse, Meinersbur.
kiranchandramohan added a subscriber: klausler.
kiranchandramohan added a comment.

Adding Peter for expert opinion. Removing some reviewers (Alex, Michael) who are probably not interested in this patch.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1238
+template <typename T>
+void OmpStructureChecker::CheckAtomicConstructStructure(T &construct) {
+  const auto &dir{std::get<parser::Verbatim>(construct.t)};
----------------
NimishMishra wrote:
> kiranchandramohan wrote:
> > Is the template required here? Can you pass the AssignmentStatement instead here?
> It's possible. However, it would require extracting //Verbatim// and //AssignmentStmt// in //void OmpStructureChecker::Enter(const parser::OpenMPAtomicConstruct &x)// at two places: //parser::OmpAtomicUpdate// and //parser::OmpAtomic//
> 
> I put a template in this so that for both //OmpAtomicUpdate// and //OmpAtomic//, we have refactored code where the directive and assignment statement extractions are present in source code at one place. 
> 
> If there is no problem with duplication of these two extractions, I will remove this template and receive assignment statements instead.
The name of the function suggests it is checking something, so I think it is better for it to not update state. So let us move the following code back to the parent function, remove the template and only pass the assignment statement.
```
  const auto &dir{std::get<parser::Verbatim>(construct.t)};
  PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_atomic);
```


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1315-1319
+    if constexpr (common::HasMember<T, AllowedBinaryOperators>) {
+      return true;
+    } else {
+      return false;
+    }
----------------
Would `return common::HasMember<T, AllowedBinaryOperators>;` work here?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1337-1338
+                std::get<parser::ProcedureDesignator>(x.value().v.t)};
+            const auto &argSpecs{
+                std::get<std::list<parser::ActualArgSpec>>(x.value().v.t)};
+            const parser::Name *name{
----------------
This can be sunk into the `else if` part.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1357
+                    std::string iter_val{*it};
+                    if (var.GetSource().ToString() == iter_val) {
+                      foundMatch = true;
----------------
I think this will not probably work. Consider the example below. Maybe we should compare the symbols here.

Also in general, it might be better to work with the Evaluate Expression framework here.

Adding @klausler for an expert opinion.

```
program OmpAtomic
   use omp_lib
   type simple
     integer :: z
   end type
   real x
   integer :: y, z
   type(simple) ::s

   z = 1

  !$omp atomic
   z = IAND(s%z, 4)
end
```


================
Comment at: flang/test/Semantics/omp-atomic03.f90:41-87
+!$omp atomic
+   !ERROR: Invalid intrinsic procedure name in OpenMP ATOMIC construct UPDATE statement
+   y = MOD(y, 9)
+!$omp atomic
+   !ERROR: Invalid intrinsic procedure name in OpenMP ATOMIC construct UPDATE statement
+   x = ABS(x)
+!$omp atomic
----------------
I think you have added a lot of invalid intrinsic procedure name test. While testing more is mostly the right approach, I think in this case we don't get much out of it. Can you reduce this to one or two?


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

https://reviews.llvm.org/D110714