[flang][OpenMP][OpenACC] Use iterator_range in check-directive-struct…

[kparzysz]

…ure, NFC

The OpenMP code is already using iterator_range, lift it to the shared header file.

[llvmbot]

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Krzysztof Parzyszek (kparzysz)

Changes

…ure, NFC

The OpenMP code is already using iterator_range, lift it to the shared header file.

---

Full diff: https://github.com/llvm/llvm-project/pull/115872.diff

3 Files Affected:

• (modified) flang/lib/Semantics/check-directive-structure.h (+4-2)
• (modified) flang/lib/Semantics/check-omp-structure.cpp (+12-10)
• (modified) flang/lib/Semantics/check-omp-structure.h (-8)

diff --git a/flang/lib/Semantics/check-directive-structure.h b/flang/lib/Semantics/check-directive-structure.h
index 2a9cb785a882f8..9578922bc8874e 100644
--- a/flang/lib/Semantics/check-directive-structure.h
+++ b/flang/lib/Semantics/check-directive-structure.h
@@ -15,6 +15,8 @@
 #include "flang/Common/enum-set.h"
 #include "flang/Semantics/semantics.h"
 #include "flang/Semantics/tools.h"
+#include "llvm/ADT/iterator_range.h"
+
 #include <unordered_map>
 
 namespace Fortran::semantics {
@@ -292,10 +294,10 @@ class DirectiveStructureChecker : public virtual BaseChecker {
     return nullptr;
   }
 
-  std::pair<typename ClauseMapTy::iterator, typename ClauseMapTy::iterator>
+  llvm::iterator_range<typename ClauseMapTy::iterator>
   FindClauses(C type) {
     auto it{GetContext().clauseInfo.equal_range(type)};
-    return it;
+    return llvm::make_range(it);
   }
 
   DirectiveContext *GetEnclosingDirContext() {
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index db7c15a0e0ec9c..4c2cf701fbbb7a 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -337,7 +337,7 @@ void OmpStructureChecker::CheckMultListItems() {
   semantics::UnorderedSymbolSet listVars;
 
   // Aligned clause
-  for (auto [_, clause] : GetClauses(llvm::omp::Clause::OMPC_aligned)) {
+  for (auto [_, clause] : FindClauses(llvm::omp::Clause::OMPC_aligned)) {
     const auto &alignedClause{std::get<parser::OmpClause::Aligned>(clause->u)};
     const auto &alignedList{std::get<0>(alignedClause.v.t)};
     std::list<parser::Name> alignedNameList;
@@ -370,7 +370,7 @@ void OmpStructureChecker::CheckMultListItems() {
   }
 
   // Nontemporal clause
-  for (auto [_, clause] : GetClauses(llvm::omp::Clause::OMPC_nontemporal)) {
+  for (auto [_, clause] : FindClauses(llvm::omp::Clause::OMPC_nontemporal)) {
     const auto &nontempClause{
         std::get<parser::OmpClause::Nontemporal>(clause->u)};
     const auto &nontempNameList{nontempClause.v};
@@ -1684,7 +1684,7 @@ void OmpStructureChecker::ChecksOnOrderedAsStandalone() {
   }};
 
   // Visit the DEPEND and DOACROSS clauses.
-  for (auto [_, clause] : GetClauses(llvm::omp::Clause::OMPC_depend)) {
+  for (auto [_, clause] : FindClauses(llvm::omp::Clause::OMPC_depend)) {
     const auto &dependClause{std::get<parser::OmpClause::Depend>(clause->u)};
     if (auto *doAcross{std::get_if<parser::OmpDoacross>(&dependClause.v.u)}) {
       visitDoacross(*doAcross, clause->source);
@@ -1693,7 +1693,7 @@ void OmpStructureChecker::ChecksOnOrderedAsStandalone() {
           "Only SINK or SOURCE dependence types are allowed when ORDERED construct is a standalone construct with no ORDERED region"_err_en_US);
     }
   }
-  for (auto [_, clause] : GetClauses(llvm::omp::Clause::OMPC_doacross)) {
+  for (auto [_, clause] : FindClauses(llvm::omp::Clause::OMPC_doacross)) {
     auto &doaClause{std::get<parser::OmpClause::Doacross>(clause->u)};
     visitDoacross(doaClause.v.v, clause->source);
   }
@@ -1734,13 +1734,13 @@ void OmpStructureChecker::CheckOrderedDependClause(
       }
     }
   }};
-  for (auto [_, clause] : GetClauses(llvm::omp::Clause::OMPC_depend)) {
+  for (auto [_, clause] : FindClauses(llvm::omp::Clause::OMPC_depend)) {
     auto &dependClause{std::get<parser::OmpClause::Depend>(clause->u)};
     if (auto *doAcross{std::get_if<parser::OmpDoacross>(&dependClause.v.u)}) {
       visitDoacross(*doAcross, clause->source);
     }
   }
-  for (auto [_, clause] : GetClauses(llvm::omp::Clause::OMPC_doacross)) {
+  for (auto [_, clause] : FindClauses(llvm::omp::Clause::OMPC_doacross)) {
     auto &doaClause{std::get<parser::OmpClause::Doacross>(clause->u)};
     visitDoacross(doaClause.v.v, clause->source);
   }
@@ -3820,7 +3820,7 @@ void OmpStructureChecker::Enter(const parser::OmpClause::UseDevicePtr &x) {
   SymbolSourceMap currSymbols;
   GetSymbolsInObjectList(x.v, currSymbols);
   semantics::UnorderedSymbolSet listVars;
-  for (auto [_, clause] : GetClauses(llvm::omp::Clause::OMPC_use_device_ptr)) {
+  for (auto [_, clause] : FindClauses(llvm::omp::Clause::OMPC_use_device_ptr)) {
     const auto &useDevicePtrClause{
         std::get<parser::OmpClause::UseDevicePtr>(clause->u)};
     const auto &useDevicePtrList{useDevicePtrClause.v};
@@ -3849,7 +3849,8 @@ void OmpStructureChecker::Enter(const parser::OmpClause::UseDeviceAddr &x) {
   SymbolSourceMap currSymbols;
   GetSymbolsInObjectList(x.v, currSymbols);
   semantics::UnorderedSymbolSet listVars;
-  for (auto [_, clause] : GetClauses(llvm::omp::Clause::OMPC_use_device_addr)) {
+  for (auto [_, clause] :
+      FindClauses(llvm::omp::Clause::OMPC_use_device_addr)) {
     const auto &useDeviceAddrClause{
         std::get<parser::OmpClause::UseDeviceAddr>(clause->u)};
     const auto &useDeviceAddrList{useDeviceAddrClause.v};
@@ -3871,7 +3872,7 @@ void OmpStructureChecker::Enter(const parser::OmpClause::IsDevicePtr &x) {
   SymbolSourceMap currSymbols;
   GetSymbolsInObjectList(x.v, currSymbols);
   semantics::UnorderedSymbolSet listVars;
-  for (auto [_, clause] : GetClauses(llvm::omp::Clause::OMPC_is_device_ptr)) {
+  for (auto [_, clause] : FindClauses(llvm::omp::Clause::OMPC_is_device_ptr)) {
     const auto &isDevicePtrClause{
         std::get<parser::OmpClause::IsDevicePtr>(clause->u)};
     const auto &isDevicePtrList{isDevicePtrClause.v};
@@ -3903,7 +3904,8 @@ void OmpStructureChecker::Enter(const parser::OmpClause::HasDeviceAddr &x) {
   SymbolSourceMap currSymbols;
   GetSymbolsInObjectList(x.v, currSymbols);
   semantics::UnorderedSymbolSet listVars;
-  for (auto [_, clause] : GetClauses(llvm::omp::Clause::OMPC_has_device_addr)) {
+  for (auto [_, clause] :
+      FindClauses(llvm::omp::Clause::OMPC_has_device_addr)) {
     const auto &hasDeviceAddrClause{
         std::get<parser::OmpClause::HasDeviceAddr>(clause->u)};
     const auto &hasDeviceAddrList{hasDeviceAddrClause.v};
diff --git a/flang/lib/Semantics/check-omp-structure.h b/flang/lib/Semantics/check-omp-structure.h
index 0c5f97f743e2e1..8c13dd20d1e399 100644
--- a/flang/lib/Semantics/check-omp-structure.h
+++ b/flang/lib/Semantics/check-omp-structure.h
@@ -141,9 +141,6 @@ class OmpStructureChecker
 #include "llvm/Frontend/OpenMP/OMP.inc"
 
 private:
-  inline llvm::iterator_range<typename ClauseMapTy::iterator> GetClauses(
-      llvm::omp::Clause clauseId);
-
   bool CheckAllowedClause(llvmOmpClause clause);
   bool IsVariableListItem(const Symbol &sym);
   bool IsExtendedListItem(const Symbol &sym);
@@ -294,10 +291,5 @@ const T *OmpStructureChecker::FindDuplicateEntry(const std::list<T> &list) {
   return nullptr;
 }
 
-llvm::iterator_range<typename OmpStructureChecker::ClauseMapTy::iterator>
-OmpStructureChecker::GetClauses(llvm::omp::Clause clauseId) {
-  return llvm::make_range(FindClauses(clauseId));
-}
-
 } // namespace Fortran::semantics
 #endif // FORTRAN_SEMANTICS_CHECK_OMP_STRUCTURE_H_

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[erichkeane]

I don't have a problem with this, plus it isn't really my area of the code. That said, I find myself wondering why .equal_range is using a pair rather than an iterator range itself?

Also as a nit, we are supposed to use camelCase for function names.

[kparzysz]

I don't have a problem with this, plus it isn't really my area of the code. That said, I find myself wondering why .equal_range is using a pair rather than an iterator range itself?

It's a member function of std::multimap.

Also as a nit, we are supposed to use camelCase for function names.

Flang uses different conventions, I'm just keeping it in sync with other functions in this file...