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

[Peixin Qiao via Phabricator via llvm-commits]

peixin accepted this revision.
peixin added a comment.

LGTM



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1310
+      context_.Say(variable.GetSource(),
+          "Atomic update variable '%s' not found in the RHS of the assignment statement in an ATOMIC (UPDATE) construct"_err_en_US,
+          variableName);
----------------
Nit: It's strange that clang-tidy does not report the error for this.
          "Atomic update variable '%s' not found in the RHS of the assignment "
          "statement in an ATOMIC (UPDATE) construct"_err_en_US,


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1332
+                    name->source == "iand" || name->source == "ior" ||
+                    name->source == "ieor")) {
+              context_.Say(expr.source,
----------------
Nit: Should we use the following format?
                !(name->source == "max" || name->source == "min" ||
                  name->source == "iand" || name->source == "ior" ||
                  name->source == "ieor")) {


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1334
+              context_.Say(expr.source,
+                  "Invalid intrinsic procedure name in OpenMP ATOMIC (UPDATE) statement"_err_en_US);
+            } else if (name) {
----------------
Nit: https://llvm.org/docs/CodingStandards.html#source-code-width
                  "Invalid intrinsic procedure name in OpenMP ATOMIC (UPDATE) "
                  "statement"_err_en_US);


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1360
+                context_.Say(expr.source,
+                    "Atomic update variable '%s' not found in the argument list of intrinsic procedure"_err_en_US,
+                    var.GetSource().ToString());
----------------
Nit: -> 80 columns?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1398
+      if (numMemoryOrderClause > 1) {
+        context_.Say(clause.source,
+            "More than one memory order clause not allowed on OpenMP Atomic construct"_err_en_US);
----------------
Using lambda for the error message may be better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110714