[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

1619

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

1619

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

[kiranchandramohan]

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

1598–1603

Add tests for these.

1619

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

285

Nit: Remove the empty line.

286

Is std::set better?

291

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

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:
   void CheckPredefinedAllocatorRestriction(const parser::CharBlock &source,
       const parser::OmpObjectList &ompObjectList);
   void CheckPredefinedAllocatorRestriction(
       const parser::CharBlock &source, const parser::Name &name);
   bool isPredefinedAllocator{false};
   void EnterDirectiveNest(const int index) { directiveNest_[index]++; }
   void ExitDirectiveNest(const int index) { directiveNest_[index]--; }
   int GetDirectiveNest(const int index) { return directiveNest_[index]; }
+  template <typename D> void CheckHintClause(D *, D *);
 
   enum directiveNestType {
     SIMDNest,
     TargetBlockOnlyTeams,
     TargetNest,
     LastType
   };
+
+  enum HintValues {
+    hint_none = 0x0,
+    hint_uncontended = 0x1,
+    hint_contended = 0x2,
+    hint_nonspeculative = 0x4,
+    hint_speculative = 0x8
+  };
+
+  std::list<int> AllowedHintValues = {hint_none, hint_uncontended,
+      hint_contended, hint_nonspeculative, hint_speculative,
+      hint_uncontended + hint_nonspeculative,
+      hint_uncontended + hint_speculative, hint_contended + hint_nonspeculative,
+      hint_contended + hint_speculative};
   int directiveNest_[LastType + 1] = {0};
 };
 } // namespace Fortran::semantics
 #endif // FORTRAN_SEMANTICS_CHECK_OMP_STRUCTURE_H_

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

common::visit(
             [&](const parser::Name &name) {
               CheckPredefinedAllocatorRestriction(source, name);
             },
         },
         ompObject.u);
   }
 }
 
+template <typename D>
+void OmpStructureChecker::CheckHintClause(
+    D *leftOmpClauseList, D *rightOmpClauseList) {
+
+  auto checkForValidHintClause = [&](const D *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 (auto it{std::find(AllowedHintValues.begin(),
+                    AllowedHintValues.end(), hintValue)};
+                it == AllowedHintValues.end())
+              context_.Say(clause.source,
+                  "Hint clause value "
+                  "is not a valid OpenMP synchronization value"_err_en_US);
+          } else {
+            context_.Say(clause.source,
+                "Hint clause must have non-negative constant "
+                "integer expression"_err_en_US);
+          }
+        }
+      }
+    }
+  };
+
+  if (leftOmpClauseList) {
+    checkForValidHintClause(leftOmpClauseList);
+  }
+  if (rightOmpClauseList) {
+    checkForValidHintClause(rightOmpClauseList);
+  }
+}
+
 void OmpStructureChecker::Enter(const parser::OpenMPConstruct &x) {
   // Simd Construct with Ordered Construct Nesting check
   // We cannot use CurrentDirectiveIsNested() here because
   // PushContextAndClauseSets() has not been called yet, it is
   // called individually for each construct.  Therefore a
   // dirContext_ size `1` means the current construct is nested
   if (dirContext_.size() >= 1) {
     if (GetDirectiveNest(SIMDNest) > 0) {
auto checkForValidMemoryOrderClause =
             if (numMemoryOrderClause > 1) {
               context_.Say(clause.source,
                   "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::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),
                 nullptr);
+            CheckHintClause<const parser::OmpAtomicClauseList>(
+                &std::get<parser::OmpAtomicClauseList>(atomicConstruct.t),
+                nullptr);
           },
           [&](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>>(
                     atomicUpdate.t)
                     .statement);
             CheckAtomicMemoryOrderClause(
                 &std::get<0>(atomicUpdate.t), &std::get<2>(atomicUpdate.t));
+            CheckHintClause<const parser::OmpAtomicClauseList>(
+                &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));
+            CheckHintClause<const parser::OmpAtomicClauseList>(
+                &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
+    logical :: z
+    real :: k
+    integer :: p(1)
+    !$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
+    
+    !$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: Hint clause must have non-negative constant integer expression
+    !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
+    
+    !ERROR: Hint clause must have non-negative constant integer expression
+    !ERROR: Must be a constant value
+    !$omp atomic hint(omp_sync_hint_uncontended + omp_sync_hint) read
+        y = x 
+
+    !$omp atomic hint(omp_sync_hint_nonspeculative)
+        y = y * 9
+
+    !$omp atomic hint(omp_sync_hint_none) read
+        y = x
+
+    !$omp atomic read hint(omp_sync_hint_uncontended + omp_lock_hint_speculative)
+        y = x
+
+    !$omp atomic hint(omp_lock_hint_nonspeculative + omp_lock_hint_uncontended)
+        x = x * y
+
+    !$omp atomic write hint(omp_lock_hint_contended + omp_sync_hint_speculative)
+        x = 10 * y
+
+    !$omp atomic hint(omp_lock_hint_contended + omp_sync_hint_nonspeculative)
+        x = y + x
+
+    !ERROR: Hint clause value is not a valid OpenMP synchronization value
+    !$omp atomic hint(omp_sync_hint_uncontended + omp_sync_hint_contended) read
+        y = x 
+
+    !ERROR: Hint clause value is not a valid OpenMP synchronization value
+    !$omp atomic hint(omp_sync_hint_nonspeculative + omp_lock_hint_speculative)
+        y = y * 9
+
+    !ERROR: Hint clause must have non-negative constant integer expression
+    !$omp atomic hint(1.0) read
+        y = x
+
+    !ERROR: Hint clause must have non-negative constant integer expression
+    !ERROR: Operands of + must be numeric; have LOGICAL(4) and INTEGER(4)
+    !$omp atomic hint(z + omp_sync_hint_nonspeculative) read
+        y = x
+
+    !ERROR: Hint clause must have non-negative constant integer expression
+    !ERROR: Must be a constant value
+    !$omp atomic hint(k + omp_sync_hint_speculative) read
+        y = x
+
+    !ERROR: Hint clause must have non-negative constant integer expression
+    !ERROR: Must be a constant value
+    !$omp atomic hint(p(1) + omp_sync_hint_uncontended) write
+        x = 10 * y
+end program