Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding parsing and semantic check support for omp masked #91432

Merged
merged 6 commits into from
May 21, 2024

Conversation

anchuraj
Copy link
Contributor

@anchuraj anchuraj commented May 8, 2024

omp masked directive in OpenMP 5.2 allows to specify code regions which are expected to be executed by thread ids specified by the programmer. Filter clause of the directive allows to specify the thread id. This change adds the parsing support for the directive

@llvmbot
Copy link
Collaborator

llvmbot commented May 8, 2024

@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-flang-parser
@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Anchu Rajendran S (anchuraj)

Changes

omp masked directive in OpenMP 5.2 allows to specify code regions which are expected to be executed by thread ids specified by the programmer. Filter clause of the directive allows to specify the thread id. This change adds the parsing support for the directive


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

5 Files Affected:

  • (modified) flang/lib/Parser/openmp-parsers.cpp (+14-5)
  • (modified) flang/lib/Parser/unparse.cpp (+18)
  • (modified) flang/lib/Semantics/resolve-directives.cpp (+49-12)
  • (added) flang/test/Parser/OpenMP/masked-unparse.f90 (+92)
  • (added) flang/test/Semantics/OpenMP/masked.f90 (+13)
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 48f213794247d..e470bf7856607 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -220,12 +220,11 @@ TYPE_PARSER(construct<OmpAlignedClause>(
 
 // 2.9.5 ORDER ([order-modifier :]concurrent)
 TYPE_PARSER(construct<OmpOrderModifier>(
-    "REPRODUCIBLE" >> pure(OmpOrderModifier::Kind::Reproducible)) ||
+                "REPRODUCIBLE" >> pure(OmpOrderModifier::Kind::Reproducible)) ||
     construct<OmpOrderModifier>(
-    "UNCONSTRAINED" >> pure(OmpOrderModifier::Kind::Unconstrained)))
+        "UNCONSTRAINED" >> pure(OmpOrderModifier::Kind::Unconstrained)))
 
-TYPE_PARSER(construct<OmpOrderClause>(
-    maybe(Parser<OmpOrderModifier>{} / ":"),
+TYPE_PARSER(construct<OmpOrderClause>(maybe(Parser<OmpOrderModifier>{} / ":"),
     "CONCURRENT" >> pure(OmpOrderClause::Type::Concurrent)))
 
 TYPE_PARSER(
@@ -266,6 +265,8 @@ TYPE_PARSER(
         construct<OmpClause>(construct<OmpClause::DynamicAllocators>()) ||
     "ENTER" >> construct<OmpClause>(construct<OmpClause::Enter>(
                    parenthesized(Parser<OmpObjectList>{}))) ||
+    "FILTER" >> construct<OmpClause>(construct<OmpClause::Filter>(
+                    parenthesized(scalarIntExpr))) ||
     "FINAL" >> construct<OmpClause>(construct<OmpClause::Final>(
                    parenthesized(scalarLogicalExpr))) ||
     "FULL" >> construct<OmpClause>(construct<OmpClause::Full>()) ||
@@ -486,9 +487,17 @@ TYPE_PARSER(
     endOfLine)
 
 // Directives enclosing structured-block
-TYPE_PARSER(construct<OmpBlockDirective>(first(
+TYPE_PARSER(construct<OmpBlockDirective>(first("MASKED TASKLOOP SIMD" >>
+        pure(llvm::omp::Directive::OMPD_masked_taskloop_simd),
+    "MASKED TASKLOOP" >> pure(llvm::omp::Directive::OMPD_masked_taskloop),
+    "MASKED" >> pure(llvm::omp::Directive::OMPD_masked),
     "MASTER" >> pure(llvm::omp::Directive::OMPD_master),
     "ORDERED" >> pure(llvm::omp::Directive::OMPD_ordered),
+    "PARALLEL MASKED TASKLOOP SIMD" >>
+        pure(llvm::omp::Directive::OMPD_parallel_masked_taskloop_simd),
+    "PARALLEL MASKED TASKLOOP" >>
+        pure(llvm::omp::Directive::OMPD_parallel_masked_taskloop),
+    "PARALLEL MASKED" >> pure(llvm::omp::Directive::OMPD_parallel_masked),
     "PARALLEL WORKSHARE" >> pure(llvm::omp::Directive::OMPD_parallel_workshare),
     "PARALLEL" >> pure(llvm::omp::Directive::OMPD_parallel),
     "SINGLE" >> pure(llvm::omp::Directive::OMPD_single),
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 3398b395f198f..7b6b083cf3e62 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2283,12 +2283,30 @@ class UnparseVisitor {
   }
   void Unparse(const OmpBlockDirective &x) {
     switch (x.v) {
+    case llvm::omp::Directive::OMPD_masked_taskloop_simd:
+      Word("MASKED TASKLOOP SIMD");
+      break;
+    case llvm::omp::Directive::OMPD_masked_taskloop:
+      Word("MASKED TASKLOOP");
+      break;
+    case llvm::omp::Directive::OMPD_masked:
+      Word("MASKED");
+      break;
     case llvm::omp::Directive::OMPD_master:
       Word("MASTER");
       break;
     case llvm::omp::Directive::OMPD_ordered:
       Word("ORDERED ");
       break;
+    case llvm::omp::Directive::OMPD_parallel_masked_taskloop_simd:
+      Word("PARALLEL MASKED TASKLOOP SIMD");
+      break;
+    case llvm::omp::Directive::OMPD_parallel_masked_taskloop:
+      Word("PARALLEL MASKED TASKLOOP");
+      break;
+    case llvm::omp::Directive::OMPD_parallel_masked:
+      Word("PARALLEL MASKED");
+      break;
     case llvm::omp::Directive::OMPD_parallel_workshare:
       Word("PARALLEL WORKSHARE ");
       break;
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 2add2056f658d..b11fa3174277d 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -19,8 +19,10 @@
 #include "flang/Parser/parse-tree.h"
 #include "flang/Parser/tools.h"
 #include "flang/Semantics/expression.h"
+#include <cstdint>
 #include <list>
 #include <map>
+#include <optional>
 #include <sstream>
 
 template <typename T>
@@ -50,6 +52,7 @@ template <typename T> class DirectiveAttributeVisitor {
     Symbol::Flag defaultDSA{Symbol::Flag::AccShared}; // TODOACC
     std::map<const Symbol *, Symbol::Flag> objectWithDSA;
     bool withinConstruct{false};
+    std::optional<int64_t> maskedTId;
     std::int64_t associatedLoopLevel{0};
   };
 
@@ -90,6 +93,9 @@ template <typename T> class DirectiveAttributeVisitor {
   void SetContextAssociatedLoopLevel(std::int64_t level) {
     GetContext().associatedLoopLevel = level;
   }
+  void SetMaskedTId(std::optional<int64_t> tid) {
+    GetContext().maskedTId = tid;
+  }
   Symbol &MakeAssocSymbol(const SourceName &name, Symbol &prev, Scope &scope) {
     const auto pair{scope.try_emplace(name, Attrs{}, HostAssocDetails{prev})};
     return *pair.first->second;
@@ -646,6 +652,7 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
 
 private:
   std::int64_t GetAssociatedLoopLevelFromClauses(const parser::OmpClauseList &);
+  std::optional<int64_t> GetMaskedTId(const parser::OmpClauseList &);
 
   Symbol::Flags dataSharingAttributeFlags{Symbol::Flag::OmpShared,
       Symbol::Flag::OmpPrivate, Symbol::Flag::OmpFirstPrivate,
@@ -1105,18 +1112,18 @@ bool AccAttributeVisitor::Pre(const parser::OpenACCCombinedConstruct &x) {
 static bool IsLastNameArray(const parser::Designator &designator) {
   const auto &name{GetLastName(designator)};
   const evaluate::DataRef dataRef{*(name.symbol)};
-  return common::visit(
-      common::visitors{
-          [](const evaluate::SymbolRef &ref) {
-            return ref->Rank() > 0 ||
-                ref->GetType()->category() == DeclTypeSpec::Numeric;
-          },
-          [](const evaluate::ArrayRef &aref) {
-            return aref.base().IsSymbol() ||
-                aref.base().GetComponent().base().Rank() == 0;
-          },
-          [](const auto &) { return false; },
-      },
+  return common::visit(common::visitors{
+                           [](const evaluate::SymbolRef &ref) {
+                             return ref->Rank() > 0 ||
+                                 ref->GetType()->category() ==
+                                 DeclTypeSpec::Numeric;
+                           },
+                           [](const evaluate::ArrayRef &aref) {
+                             return aref.base().IsSymbol() ||
+                                 aref.base().GetComponent().base().Rank() == 0;
+                           },
+                           [](const auto &) { return false; },
+                       },
       dataRef.u);
 }
 
@@ -1498,11 +1505,35 @@ void AccAttributeVisitor::CheckMultipleAppearances(
     AddDataSharingAttributeObject(*target);
   }
 }
+std::optional<int64_t> OmpAttributeVisitor::GetMaskedTId(
+    const parser::OmpClauseList &clauseList) {
+  for (const auto &clause : clauseList.v) {
+    if (const auto *filterClause{
+            std::get_if<parser::OmpClause::Filter>(&clause.u)}) {
+      if (const auto v{EvaluateInt64(context_, filterClause->v)}) {
+        return v;
+      }
+    }
+  }
+  // if no thread id is specified in filter clause, the masked thread id should
+  // be master's
+  return 0;
+}
 
 bool OmpAttributeVisitor::Pre(const parser::OpenMPBlockConstruct &x) {
   const auto &beginBlockDir{std::get<parser::OmpBeginBlockDirective>(x.t)};
   const auto &beginDir{std::get<parser::OmpBlockDirective>(beginBlockDir.t)};
+  const auto &clauseList{std::get<parser::OmpClauseList>(beginBlockDir.t)};
   switch (beginDir.v) {
+  case llvm::omp::Directive::OMPD_masked_taskloop_simd:
+  case llvm::omp::Directive::OMPD_masked_taskloop:
+  case llvm::omp::Directive::OMPD_masked:
+  case llvm::omp::Directive::OMPD_parallel_masked_taskloop_simd:
+  case llvm::omp::Directive::OMPD_parallel_masked_taskloop:
+  case llvm::omp::Directive::OMPD_parallel_masked:
+    PushContext(beginDir.source, beginDir.v);
+    SetMaskedTId(GetMaskedTId(clauseList));
+    break;
   case llvm::omp::Directive::OMPD_master:
   case llvm::omp::Directive::OMPD_ordered:
   case llvm::omp::Directive::OMPD_parallel:
@@ -1532,6 +1563,12 @@ void OmpAttributeVisitor::Post(const parser::OpenMPBlockConstruct &x) {
   const auto &beginBlockDir{std::get<parser::OmpBeginBlockDirective>(x.t)};
   const auto &beginDir{std::get<parser::OmpBlockDirective>(beginBlockDir.t)};
   switch (beginDir.v) {
+  case llvm::omp::Directive::OMPD_masked_taskloop_simd:
+  case llvm::omp::Directive::OMPD_masked_taskloop:
+  case llvm::omp::Directive::OMPD_masked:
+  case llvm::omp::Directive::OMPD_parallel_masked_taskloop_simd:
+  case llvm::omp::Directive::OMPD_parallel_masked_taskloop:
+  case llvm::omp::Directive::OMPD_parallel_masked:
   case llvm::omp::Directive::OMPD_parallel:
   case llvm::omp::Directive::OMPD_single:
   case llvm::omp::Directive::OMPD_target:
diff --git a/flang/test/Parser/OpenMP/masked-unparse.f90 b/flang/test/Parser/OpenMP/masked-unparse.f90
new file mode 100644
index 0000000000000..96ccc3be238c0
--- /dev/null
+++ b/flang/test/Parser/OpenMP/masked-unparse.f90
@@ -0,0 +1,92 @@
+! RUN: %flang_fc1 -fdebug-unparse -fopenmp %s | FileCheck --ignore-case %s
+! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp %s | FileCheck --check-prefix="PARSE-TREE" %s
+
+! Check for parsing of masked directive with filter clause.
+
+
+subroutine test_masked()
+  integer :: c = 1
+  !PARSE-TREE: OmpBeginBlockDirective
+  !PARSE-TREE-NEXT: OmpBlockDirective -> llvm::omp::Directive = masked
+  !CHECK: !$omp masked
+  !$omp masked
+  c = c + 1
+  !$omp end masked
+  !PARSE-TREE: OmpBeginBlockDirective
+  !PARSE-TREE-NEXT: OmpBlockDirective -> llvm::omp::Directive = masked
+  !PARSE-TREE-NEXT: OmpClauseList -> OmpClause -> Filter -> Scalar -> Integer -> Expr = '1_4'
+  !PARSE-TREE-NEXT: LiteralConstant -> IntLiteralConstant = '1'
+  !CHECK: !$omp masked filter(1_4)
+  !$omp masked filter(1)
+  c = c + 2
+  !$omp end masked
+end subroutine
+
+subroutine test_masked_taskloop_simd()
+  integer :: i, j = 1
+  !PARSE-TREE: OmpBeginBlockDirective
+  !PARSE-TREE-NEXT: OmpBlockDirective -> llvm::omp::Directive = masked taskloop simd
+  !CHECK: !$omp masked taskloop simd
+  !$omp masked taskloop simd
+  do i=1,10
+   j = j + 1
+  end do
+  !$omp end masked taskloop simd
+end subroutine
+
+subroutine test_masked_taskloop
+  integer :: i, j = 1
+  !PARSE-TREE: OmpBeginBlockDirective
+  !PARSE-TREE-NEXT: OmpBlockDirective -> llvm::omp::Directive = masked taskloop
+  !PARSE-TREE-NEXT: OmpClauseList -> OmpClause -> Filter -> Scalar -> Integer -> Expr = '2_4'
+  !PARSE-TREE-NEXT: LiteralConstant -> IntLiteralConstant = '2'
+  !CHECK: !$omp masked taskloop filter(2_4)
+  !$omp masked taskloop filter(2)
+  do i=1,10
+   j = j + 1
+  end do
+  !$omp end masked taskloop
+end subroutine
+
+subroutine test_parallel_masked
+  integer, parameter :: i = 1, j = 1
+  integer :: c = 2
+  !PARSE-TREE: OmpBeginBlockDirective
+  !PARSE-TREE-NEXT: OmpBlockDirective -> llvm::omp::Directive = parallel masked
+  !PARSE-TREE-NEXT: OmpClauseList -> OmpClause -> Filter -> Scalar -> Integer -> Expr = '2_4'
+  !PARSE-TREE-NEXT: Add
+  !PARSE-TREE-NEXT: Expr = '1_4'
+  !PARSE-TREE-NEXT: Designator -> DataRef -> Name = 'i'
+  !PARSE-TREE-NEXT: Expr = '1_4'
+  !PARSE-TREE-NEXT: Designator -> DataRef -> Name = 'j'
+  !CHECK: !$omp parallel masked filter(2_4)
+  !$omp parallel masked filter(i+j)
+  c = c + 2
+  !$omp end parallel masked
+end subroutine
+
+subroutine test_parallel_masked_taskloop_simd
+  integer :: i, j = 1
+  !PARSE-TREE: OmpBeginBlockDirective
+  !PARSE-TREE-NEXT: OmpBlockDirective -> llvm::omp::Directive = parallel masked taskloop simd
+  !CHECK: !$omp parallel masked taskloop simd
+  !$omp parallel masked taskloop simd
+  do i=1,10
+   j = j + 1
+  end do
+  !$omp end parallel masked taskloop simd
+end subroutine
+
+subroutine test_parallel_masked_taskloop
+  integer :: i, j = 1
+  !PARSE-TREE: OmpBeginBlockDirective
+  !PARSE-TREE-NEXT: OmpBlockDirective -> llvm::omp::Directive = parallel masked taskloop
+  !PARSE-TREE-NEXT: OmpClauseList -> OmpClause -> Filter -> Scalar -> Integer -> Expr = '2_4'
+  !PARSE-TREE-NEXT: LiteralConstant -> IntLiteralConstant = '2'
+  !CHECK: !$omp parallel masked taskloop filter(2_4)
+  !$omp parallel masked taskloop filter(2)
+  do i=1,10
+   j = j + 1
+  end do
+  !$omp end parallel masked taskloop
+end subroutine
diff --git a/flang/test/Semantics/OpenMP/masked.f90 b/flang/test/Semantics/OpenMP/masked.f90
new file mode 100644
index 0000000000000..36e22ee0be8c5
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/masked.f90
@@ -0,0 +1,13 @@
+! RUN: %python %S/../test_errors.py %s %flang_fc1 -fopenmp
+
+subroutine test_masked()
+  integer :: c = 1
+  !ERROR: At most one FILTER clause can appear on the MASKED directive
+  !$omp masked filter(1) filter(2)
+  c = c + 1
+  !$omp end masked
+  !ERROR: NOWAIT clause is not allowed on the MASKED directive
+  !$omp masked nowait
+  c = c + 2
+  !$omp end masked
+end subroutine

Copy link
Contributor

@skatrak skatrak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Anchu, this looks mostly good to me. I just have a couple of comments mostly to generally reduce the scope of the changes to only what's necessary.

flang/lib/Parser/openmp-parsers.cpp Outdated Show resolved Hide resolved
flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved
flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved
flang/lib/Parser/openmp-parsers.cpp Outdated Show resolved Hide resolved
flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved
flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved
flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved
flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this patch.

Could you add a relevant TODO message in lowering so that any test using masked will not crash. We are trying to stabilize the code by reducing crashes. The behaviour we are trying to get to is that the compiler either accepts the code and executes correctly or provides a TODO message saying it is not implemented yet.

@anchuraj
Copy link
Contributor Author

anchuraj commented May 8, 2024

Thank you @skatrak @mjklemm @kiranchandramohan for reviewing the PR. I have removed the unrelated formatting changes and am working on the other suuggestions.

Copy link
Contributor

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the changes. Please wait for @skatrak and/or @mjklemm.

It will help gracefully erroring out if you can make the OpenMPIRBuilder changes before implementing the Flang lowering.

flang/test/Lower/OpenMP/Todo/masked-directive.f90 Outdated Show resolved Hide resolved
Copy link
Contributor

@skatrak skatrak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my feedback, it's looking nearly ready to me. Just some minor comments.

flang/include/flang/Semantics/openmp-directive-sets.h Outdated Show resolved Hide resolved
flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved
flang/test/Lower/OpenMP/Todo/masked-directive.f90 Outdated Show resolved Hide resolved
flang/lib/Lower/OpenMP/OpenMP.cpp Outdated Show resolved Hide resolved
@mjklemm
Copy link
Contributor

mjklemm commented May 12, 2024

It might be a good idea to have the compiler emit a warning about the deprecation of the master construct and that it should be replaced with masked. Would you agree? I'm OK with doing that in this PR or a separate PR.

@anchuraj
Copy link
Contributor Author

anchuraj commented May 15, 2024

It might be a good idea to have the compiler emit a warning about the deprecation of the master construct and that it should be replaced with masked. Would you agree? I'm OK with doing that in this PR or a separate PR.

I feel such a warning would be appropriate after masked is supported, since, until then users wont be able to replace. Please let me know your thoughts.

I am working on end to end support. I will add this as a future PR, if you agree @mjklemm

Copy link

github-actions bot commented May 15, 2024

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

@mjklemm
Copy link
Contributor

mjklemm commented May 15, 2024

I am working on end to end support. I will add this as a future PR, if you agree @mjklemm

Works for me! Thanks!

@anchuraj anchuraj requested a review from skatrak May 16, 2024 05:17
@mjklemm mjklemm self-requested a review May 17, 2024 06:38
Copy link
Contributor

@mjklemm mjklemm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

Copy link
Contributor

@skatrak skatrak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Anchu, LGTM!

@anchuraj anchuraj merged commit 6658e1a into llvm:main May 21, 2024
4 checks passed
topperc added a commit to topperc/llvm-project that referenced this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants