[flang][OpenMP] Added semantic checks for hint clause

[NimishMishra]

Summary

This patch improves semantic checks for hint clause.

• check "hint-expression is a constant expression that evaluates to a scalar value with kind omp_sync_hint_kind and a value that is a valid synchronization hint."

Depends on D127822

[peixin]

Can you split this patch into two:

1. refactor the atomic code preparing for other semantic check patches
2. check hint clause for both atomic and critical construct: 2.1 At most one hint clause can be specified. 2.2 The hint clause must have the non-negative constant integer expression. (Maybe I am wrong for this?)

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

1584

I don't think the value must be 0, 1, 2, 4, 8. Please check OpenMP 5.0 2.17.12:

The hints can be combined by using the + or | operators in C/C++ or the + operator in Fortran.

You may also check the behavior of clang. The hint clause must have the non-negative constant integer expression.

[kiranchandramohan]

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

1584

+1. Also use descriptive name/ENUMs instead of hardcoded constants.

[kiranchandramohan]

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

1563–1568

Add tests for these.

1584

There are also some restrictions on what hints can be combined. Hints are present in the Critical construct it. It will be good if you can add a common check for hints when you split the patch.

The hints omp_sync_hint_uncontended and omp_sync_hint_contended cannot be combined.
The hints omp_sync_hint_nonspeculative and omp_sync_hint_speculative cannot be combined

https://www.openmp.org/spec-html/5.0/openmpsu100.html

[NimishMishra]

Revised the diff to reflect hint clause changes only

[peixin]

Can you also support this check for critical construct?

[NimishMishra]

Added support for critical construct also

[peixin]

Is hint_contended + hint_speculative + hint_speculative rejected in other compilers?

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

332

Add assert for ompClause and remove the nullptr. And no need to check hintClause.

flang/lib/Semantics/check-omp-structure.h

278

Nit: Place directiveNest_ things together. I mean, move line 293 to line 279.

286

Nit: Remove the empty line.

287

Is std::set better?

flang/test/Semantics/OpenMP/omp-atomic-hint-clause.f90

10

Please add two more scenarios:

1. integer, parameter :: a = 1
2. abs(1)

[NimishMishra]

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

332

This is problematic whenever OmpAtomicClauseList is used. This is because every OmpAtomicClause is a variant of OmpMemoryOrderClause and OmpClause. Means whenever memory order clauses are used:

!$omp atomic read release
   v = x

The compiler starts giving assertion failures.

[peixin]

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

332

OK. Now I understand how your code is organized. How about the following:

if (const Fortran::parser::OmpClause::Hint *hintClause = std::get_if<Fortran::parser::OmpClause::Hint>(&ompClause->u)) {

std::optional<std::int64_t> hintValue = GetIntValue(hintClause->v);
if (hintValue) {
  checking ...
} else {
   error
}

}

335

Nit

[NimishMishra]

• Addressed comments.

• Improved the logic to check for presence of certain bits in tandem, instead of comparing the entire hintValue to predefined integer values. Concretely, in the last nibble in the integral hintValue, 0xC and 0x3 are not allowed.

[NimishMishra]

@peixin @kiranchandramohan does this look in good shape now?

[peixin]

LGTM except for one nit.

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

339

flang/lib/Semantics/check-omp-structure.h

private:
   bool CheckTargetBlockOnlyTeams(const parser::Block &);
   void CheckWorkshareBlockStmts(const parser::Block &, parser::CharBlock);
 
   void CheckLoopItrVariableIsInt(const parser::OpenMPLoopConstruct &x);
   void CheckDoWhile(const parser::OpenMPLoopConstruct &x);
   void CheckCycleConstraints(const parser::OpenMPLoopConstruct &x);
   template <typename T, typename D> bool IsOperatorValid(const T &, const D &);
   void CheckAtomicMemoryOrderClause(
-      const parser::OmpAtomicClauseList &, const parser::OmpAtomicClauseList &);
-  void CheckAtomicMemoryOrderClause(const parser::OmpAtomicClauseList &);
+      const parser::OmpAtomicClauseList *, const parser::OmpAtomicClauseList *);
+  void CheckAtomicHintClause(
+      const parser::OmpAtomicClauseList *, const parser::OmpAtomicClauseList *);
   void CheckAtomicUpdateAssignmentStmt(const parser::AssignmentStmt &);
   void CheckAtomicConstructStructure(const parser::OpenMPAtomicConstruct &);
   void CheckDistLinear(const parser::OpenMPLoopConstruct &x);
   void CheckSIMDNest(const parser::OpenMPConstruct &x);
   void CheckTargetNest(const parser::OpenMPConstruct &x);
   void CheckCancellationNest(
       const parser::CharBlock &source, const parser::OmpCancelType::Type &type);
   std::int64_t GetOrdCollapseLevel(const parser::OpenMPLoopConstruct &x);
private:
   int GetDirectiveNest(const int index) { return directiveNest_[index]; }
 
   enum directiveNestType {
     SIMDNest,
     TargetBlockOnlyTeams,
     TargetNest,
     LastType
   };
   int directiveNest_[LastType + 1] = {0};
 };
 } // namespace Fortran::semantics
 #endif // FORTRAN_SEMANTICS_CHECK_OMP_STRUCTURE_H_

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

void OmpStructureChecker::Enter(const parser::OpenMPConstruct &x) {
   // dirContext_ size `1` means the current construct is nested
   if (dirContext_.size() >= 1) {
     if (GetDirectiveNest(SIMDNest) > 0) {
       CheckSIMDNest(x);
     }
     if (GetDirectiveNest(TargetNest) > 0) {
       CheckTargetNest(x);
     }
   }
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPLoopConstruct &x) {
   const auto &beginLoopDir{std::get<parser::OmpBeginLoopDirective>(x.t)};
   const auto &beginDir{std::get<parser::OmpLoopDirective>(beginLoopDir.t)};
 
   // check matching, End directive is optional
   if (const auto &endLoopDir{
           std::get<std::optional<parser::OmpEndLoopDirective>>(x.t)}) {
     const auto &endDir{
         std::get<parser::OmpLoopDirective>(endLoopDir.value().t)};
 
     CheckMatching<parser::OmpLoopDirective>(beginDir, endDir);
   }
 
common::visit(
                   "Invalid operator in OpenMP ATOMIC (UPDATE) statement"_err_en_US);
             }
           },
       },
       expr.u);
 }
 
 void OmpStructureChecker::CheckAtomicMemoryOrderClause(
-    const parser::OmpAtomicClauseList &clauseList) {
+    const parser::OmpAtomicClauseList *leftHandClauseList,
+    const parser::OmpAtomicClauseList *rightHandClauseList) {
   int numMemoryOrderClause = 0;
-  for (const auto &clause : clauseList.v) {
+  auto checkForValidMemoryOrderClause =
+      [&](const parser::OmpAtomicClauseList *clauseList) {
+        for (const auto &clause : clauseList->v) {
           if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
             numMemoryOrderClause++;
             if (numMemoryOrderClause > 1) {
               context_.Say(clause.source,
-            "More than one memory order clause not allowed on OpenMP "
-            "Atomic construct"_err_en_US);
+                  "More than one memory order clause not allowed on "
+                  "OpenMP Atomic construct"_err_en_US);
               return;
             }
           }
         }
+      };
+  if (leftHandClauseList) {
+    checkForValidMemoryOrderClause(leftHandClauseList);
+  }
+  if (rightHandClauseList) {
+    checkForValidMemoryOrderClause(rightHandClauseList);
+  }
 }
 
-void OmpStructureChecker::CheckAtomicMemoryOrderClause(
-    const parser::OmpAtomicClauseList &leftHandClauseList,
-    const parser::OmpAtomicClauseList &rightHandClauseList) {
-  int numMemoryOrderClause = 0;
-  for (const auto &clause : leftHandClauseList.v) {
-    if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
-      numMemoryOrderClause++;
-      if (numMemoryOrderClause > 1) {
+void OmpStructureChecker::CheckAtomicHintClause(
+    const parser::OmpAtomicClauseList *leftOmpAtomicClauseList,
+    const parser::OmpAtomicClauseList *rightOmpAtomicClauseList) {
+
+  auto checkForValidHintClause =
+      [&](const parser::OmpAtomicClauseList *clauseList) {
+        for (const auto &clause : clauseList->v) {
+          if (const auto ompClause =
+                  std::get_if<Fortran::parser::OmpClause>(&clause.u)) {
+            if (auto hintClause = std::get_if<Fortran::parser::OmpClause::Hint>(
+                    &ompClause->u)) {
+              if (auto evaluatedValue{GetIntValue(hintClause->v)}) {
+                int hintValue = evaluatedValue.value();
+                if (hintValue != 0x0 && hintValue != 0x1 && hintValue != 0x2 &&
+                    hintValue != 0x4 && hintValue != 0x8)
                   context_.Say(clause.source,
-            "More than one memory order clause not allowed on "
-            "OpenMP Atomic construct"_err_en_US);
-        return;
+                      "Hint clause value "
+                      "is not a valid OpenMP synchronization value"_err_en_US);
               }
             }
           }
-  for (const auto &clause : rightHandClauseList.v) {
-    if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
-      numMemoryOrderClause++;
-      if (numMemoryOrderClause > 1) {
-        context_.Say(clause.source,
-            "More than one memory order clause not "
-            "allowed on OpenMP Atomic construct"_err_en_US);
-        return;
         }
-    }
+      };
+
+  if (leftOmpAtomicClauseList) {
+    checkForValidHintClause(leftOmpAtomicClauseList);
+  if (rightOmpAtomicClauseList) {
+    checkForValidHintClause(rightOmpAtomicClauseList);
   }
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPAtomicConstruct &x) {
   common::visit(
       common::visitors{
           [&](const parser::OmpAtomic &atomicConstruct) {
             const auto &dir{std::get<parser::Verbatim>(atomicConstruct.t)};
             PushContextAndClauseSets(
                 dir.source, llvm::omp::Directive::OMPD_atomic);
             CheckAtomicUpdateAssignmentStmt(
                 std::get<parser::Statement<parser::AssignmentStmt>>(
                     atomicConstruct.t)
                     .statement);
             CheckAtomicMemoryOrderClause(
-                std::get<parser::OmpAtomicClauseList>(atomicConstruct.t));
+                &std::get<parser::OmpAtomicClauseList>(atomicConstruct.t),
+                nullptr);
+            CheckAtomicHintClause(
+                &std::get<parser::OmpAtomicClauseList>(atomicConstruct.t),
+                nullptr);
           },
-          [&](const parser::OmpAtomicUpdate &atomicConstruct) {
-            const auto &dir{std::get<parser::Verbatim>(atomicConstruct.t)};
+          [&](const parser::OmpAtomicUpdate &atomicUpdate) {
+            const auto &dir{std::get<parser::Verbatim>(atomicUpdate.t)};
             PushContextAndClauseSets(
                 dir.source, llvm::omp::Directive::OMPD_atomic);
             CheckAtomicUpdateAssignmentStmt(
                 std::get<parser::Statement<parser::AssignmentStmt>>(
-                    atomicConstruct.t)
+                    atomicUpdate.t)
                     .statement);
             CheckAtomicMemoryOrderClause(
-                std::get<0>(atomicConstruct.t), std::get<2>(atomicConstruct.t));
+                &std::get<0>(atomicUpdate.t), &std::get<2>(atomicUpdate.t));
+            CheckAtomicHintClause(
+                &std::get<0>(atomicUpdate.t), &std::get<2>(atomicUpdate.t));
           },
           [&](const auto &atomicConstruct) {
             const auto &dir{std::get<parser::Verbatim>(atomicConstruct.t)};
             PushContextAndClauseSets(
                 dir.source, llvm::omp::Directive::OMPD_atomic);
-            CheckAtomicMemoryOrderClause(
-                std::get<0>(atomicConstruct.t), std::get<2>(atomicConstruct.t));
+            CheckAtomicMemoryOrderClause(&std::get<0>(atomicConstruct.t),
+                &std::get<2>(atomicConstruct.t));
+            CheckAtomicHintClause(&std::get<0>(atomicConstruct.t),
+                &std::get<2>(atomicConstruct.t));
           },
       },
       x.u);
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPAtomicConstruct &) {
   dirContext_.pop_back();
 }

flang/test/Semantics/OpenMP/omp-atomic-hint-clause.f90

+! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp 
+! Semantic checks on hint clauses, as they appear on atomic constructs
+
+program sample
+    use omp_lib
+    integer :: x, y
+    !$omp atomic hint(1) write
+        y = 2
+    
+    !$omp atomic read hint(2)
+        y = x    
+    
+    !ERROR: Hint clause value is not a valid OpenMP synchronization value
+    !$omp atomic hint(3)
+        y = y + 10
+    
+    !ERROR: Hint clause value is not a valid OpenMP synchronization value
+    !$omp atomic update hint(5)
+        y = x
+    
+    !ERROR: Hint clause value is not a valid OpenMP synchronization value
+    !$omp atomic hint(7) capture
+        y = x
+        x = y
+    !$omp end atomic
+    
+    !ERROR: Must be a constant value
+    !$omp atomic update hint(x)
+        y = y * 1
+    
+    !$omp atomic read hint(4)
+        y = x
+
+    !$omp atomic hint(8)
+        x = x * y
+
+    !$omp atomic write hint(omp_sync_hint_uncontended)
+        x = 10 * y
+
+    !$omp atomic hint(omp_lock_hint_speculative)
+        x = y + x
+
+    !$omp atomic hint(omp_sync_hint_uncontended) read
+        y = x 
+
+    !$omp atomic hint(omp_sync_hint_nonspeculative)
+        y = y * 9
+end program