[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

1624

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

1624

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

[kiranchandramohan]

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

1603–1608

Add tests for these.

1624

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

330

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

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

279

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

287

Nit: Remove the empty line.

288

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

330

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

330

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
}

}

333

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

337

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
   };
   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 <class D>
+void OmpStructureChecker::CheckHintClause(
+    D *leftOmpClauseList, D *rightOmpClauseList) {
+  auto checkForValidHintClause = [&](const D *clauseList) {
+    for (const auto &clause : clauseList->v) {
+      const Fortran::parser::OmpClause *ompClause = nullptr;
+      if constexpr (std::is_same_v<D,
+                        const Fortran::parser::OmpAtomicClauseList>) {
+        ompClause = std::get_if<Fortran::parser::OmpClause>(&clause.u);
+        if (!ompClause)
+          continue;
+      } else if constexpr (std::is_same_v<D,
+                               const Fortran::parser::OmpClauseList>) {
+        ompClause = &clause;
+      }
+      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 && hintValue.value() >= 0) {
+          if((hintValue.value() & 0xC) == 0xC /*`omp_sync_hint_nonspeculative` and `omp_lock_hint_speculative`*/ 
+                  || (hintValue.value() & 0x3) == 0x3 /*`omp_sync_hint_uncontended` and omp_sync_hint_contended*/ )
+            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) {
context_
         .Attach(endDirName->source, "should be NULL"_en_US);
   }
   if (!dirName && !ompClause.source.empty() &&
       ompClause.source.NULTerminatedToString() != "hint(omp_sync_hint_none)") {
     context_.Say(dir.source,
         parser::MessageFormattedText{
             "Hint clause other than omp_sync_hint_none cannot be specified for an unnamed CRITICAL directive"_err_en_US});
   }
+  CheckHintClause<const parser::OmpClauseList>(&ompClause, nullptr);
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPCriticalConstruct &) {
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(
     const parser::OpenMPCancellationPointConstruct &x) {
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)
+    integer, parameter :: a = 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
+
+    !$omp atomic write hint(a)
+        x = y + x
+
+    !$omp atomic hint(abs(-1)) write
+        x = 7
+
+    !$omp atomic hint(omp_sync_hint_uncontended + omp_sync_hint_uncontended + omp_sync_hint_speculative) write
+        x = 7
+end program

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

+! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp 
+! Semantic checks on hint clauses, as they appear on critical construct
+
+program sample
+    use omp_lib
+    integer :: y
+    logical :: z
+    real :: k
+    integer :: p(1)
+    
+    !$omp critical (name) hint(1)
+        y = 2
+    !$omp end critical (name)
+    
+    !$omp critical (name) hint(2)
+        y = 2
+    !$omp end critical (name)
+     
+    !ERROR: Hint clause value is not a valid OpenMP synchronization value
+    !$omp critical (name) hint(3)
+        y = 2
+    !$omp end critical (name)
+    
+    !$omp critical (name)  hint(5)
+        y = 2
+    !$omp end critical (name)
+    
+    !ERROR: Hint clause value is not a valid OpenMP synchronization value
+    !$omp critical (name) hint(7)
+        y = 2
+    !$omp end critical (name)
+   
+    !ERROR: Hint clause must have non-negative constant integer expression
+    !ERROR: Must be a constant value
+    !$omp critical (name) hint(x)
+        y = 2
+    !$omp end critical (name)
+    
+    !$omp critical (name) hint(4)
+        y = 2
+    !$omp end critical (name)
+
+    !$omp critical (name) hint(8)
+        y = 2
+    !$omp end critical (name)
+
+    !$omp critical (name) hint(omp_sync_hint_uncontended)
+        y = 2
+    !$omp end critical (name)
+
+    !$omp critical (name) hint(omp_lock_hint_speculative)
+        y = 2
+    !$omp end critical (name)
+    
+    !ERROR: Hint clause must have non-negative constant integer expression
+    !ERROR: Must be a constant value
+    !$omp critical (name) hint(omp_sync_hint_uncontended + omp_sync_hint) 
+        y = 2
+    !$omp end critical (name)
+
+    !$omp critical (name) hint(omp_sync_hint_nonspeculative)
+        y = 2
+    !$omp end critical (name)
+
+     !$omp critical (name) hint(omp_sync_hint_none)
+        y = 2
+    !$omp end critical (name)
+
+    !$omp critical (name) hint(omp_sync_hint_uncontended + omp_lock_hint_speculative)
+        y = 2
+    !$omp end critical (name)
+
+    !$omp critical (name) hint(omp_lock_hint_nonspeculative + omp_lock_hint_uncontended)
+        y = 2
+    !$omp end critical (name)
+
+    !$omp critical (name) hint(omp_lock_hint_contended + omp_sync_hint_speculative)
+        y = 2
+    !$omp end critical (name)
+
+    !$omp critical (name) hint(omp_lock_hint_contended + omp_sync_hint_nonspeculative)
+        y = 2
+    !$omp end critical (name)
+
+    !ERROR: Hint clause value is not a valid OpenMP synchronization value
+     !$omp critical (name) hint(omp_sync_hint_uncontended + omp_sync_hint_contended)
+        y = 2
+    !$omp end critical (name)
+
+    !ERROR: Hint clause value is not a valid OpenMP synchronization value
+    !$omp critical (name) hint(omp_sync_hint_nonspeculative + omp_lock_hint_speculative)
+        y = 2
+    !$omp end critical (name)
+
+    !ERROR: Hint clause must have non-negative constant integer expression
+    !$omp critical (name) hint(1.0) 
+        y = 2
+    !$omp end critical (name)
+
+    !ERROR: Hint clause must have non-negative constant integer expression
+    !ERROR: Operands of + must be numeric; have LOGICAL(4) and INTEGER(4)
+    !$omp critical (name) hint(z + omp_sync_hint_nonspeculative)
+        y = 2
+    !$omp end critical (name)
+
+    !ERROR: Hint clause must have non-negative constant integer expression
+    !ERROR: Must be a constant value
+    !$omp critical (name) hint(k + omp_sync_hint_speculative)
+        y = 2
+    !$omp end critical (name)
+
+    !ERROR: Hint clause must have non-negative constant integer expression
+    !ERROR: Must be a constant value
+    !$omp critical (name) hint(p(1) + omp_sync_hint_uncontended)
+        y = 2
+    !$omp end critical (name)
+end program
+