[llvm-branch-commits] [flang] [flang][OpenMP] Move clause/object conversion to happen early, in genOMP (PR #87086)

2024-04-02 Thread Krzysztof Parzyszek via llvm-branch-commits 

kparzysz wrote:

We can switch to ArrayRef in another PR.

https://github.com/llvm/llvm-project/pull/87086
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [flang] [flang][OpenMP] Move clause/object conversion to happen early, in genOMP (PR #87086)

2024-04-02 Thread Sergio Afonso via llvm-branch-commits 


@@ -58,17 +58,15 @@ void gatherFuncAndVarSyms(
 const ObjectList , mlir::omp::DeclareTargetCaptureClause clause,
 llvm::SmallVectorImpl );
 
+int64_t getCollapseValue(const List );

skatrak wrote:

It looks like this function is only used within OpenMP.cpp, is it expected to 
be used elsewhere later? Otherwise it may make more sense to make it internal 
(`static`) to that compilation unit instead.

https://github.com/llvm/llvm-project/pull/87086
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [flang] [flang][OpenMP] Move clause/object conversion to happen early, in genOMP (PR #87086)

2024-04-02 Thread Sergio Afonso via llvm-branch-commits 


@@ -2357,44 +2332,44 @@ genOMP(Fortran::lower::AbstractConverter ,
   converter.genLocation(beginBlockDirective.source);
   const auto origDirective =
   std::get(beginBlockDirective.t).v;
-  const auto  =
-  std::get(beginBlockDirective.t);
-  const auto  =
-  std::get(endBlockDirective.t);
+  List beginClauses = makeClauses(
+  std::get(beginBlockDirective.t), 
semaCtx);
+  List endClauses = makeClauses(
+  std::get(endBlockDirective.t), semaCtx);
 
   assert(llvm::omp::blockConstructSet.test(origDirective) &&
  "Expected block construct");
 
-  for (const Fortran::parser::OmpClause  : beginClauseList.v) {
+  for (const Clause  : beginClauses) {
 mlir::Location clauseLocation = converter.genLocation(clause.source);
-if (!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if() &&
-!std::get_if()) {
+if (!std::get_if() &&

skatrak wrote:

Nit: I'd suggest taking the opportunity of already updating all these lines to 
sort them alphabetically and possibly replace `std::get_if` with 
`std::holds_alternative`, but feel free to ignore (same thing below for 
`endClauses`).

https://github.com/llvm/llvm-project/pull/87086
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [flang] [flang][OpenMP] Move clause/object conversion to happen early, in genOMP (PR #87086)

2024-04-02 Thread Sergio Afonso via llvm-branch-commits 


@@ -49,9 +49,8 @@ class ClauseProcessor {
 public:
   ClauseProcessor(Fortran::lower::AbstractConverter ,
   Fortran::semantics::SemanticsContext ,
-  const Fortran::parser::OmpClauseList )
-  : converter(converter), semaCtx(semaCtx),
-clauses(makeClauses(clauses, semaCtx)) {}
+  const List )
+  : converter(converter), semaCtx(semaCtx), clauses(clauses) {}

skatrak wrote:

Could we make the `clauses` field an `ArrayRef`? In the regular use 
pattern of this class the clause list of the caller does not go out of scope 
before the `ClauseProcessor` instance does. Maybe we can document this and 
avoid creating a local copy.

In any case this is a discussion to consider creating another patch for, not 
something to be addressed here.

https://github.com/llvm/llvm-project/pull/87086
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [flang] [flang][OpenMP] Move clause/object conversion to happen early, in genOMP (PR #87086)

2024-04-02 Thread Sergio Afonso via llvm-branch-commits 

https://github.com/skatrak edited 
https://github.com/llvm/llvm-project/pull/87086
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [flang] [flang][OpenMP] Move clause/object conversion to happen early, in genOMP (PR #87086)

2024-04-02 Thread Sergio Afonso via llvm-branch-commits 

https://github.com/skatrak approved this pull request.

Thank you Krzysztof, LGTM. I have a couple of suggestions, but this should be 
ready if you don't agree with them.

https://github.com/llvm/llvm-project/pull/87086
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [flang] [flang][OpenMP] Move clause/object conversion to happen early, in genOMP (PR #87086)

2024-04-02 Thread Sergio Afonso via llvm-branch-commits 


@@ -597,14 +599,11 @@ static void removeStoreOp(mlir::Operation *reductionOp, 
mlir::Value symVal) {
 // TODO: Generate the reduction operation during lowering instead of creating
 // and removing operations since this is not a robust approach. Also, removing
 // ops in the builder (instead of a rewriter) is probably not the best 
approach.
-static void
-genOpenMPReduction(Fortran::lower::AbstractConverter ,
-   Fortran::semantics::SemanticsContext ,
-   const Fortran::parser::OmpClauseList ) {
+static void genOpenMPReduction(Fortran::lower::AbstractConverter ,
+   Fortran::semantics::SemanticsContext ,
+   const List ) {

skatrak wrote:

Nit: I'm generally in favor of using `ArrayRef` in place of `const 
Container &` for function arguments in all `gen...` functions here and the 
constructor for `ClauseProcessor`, but if you don't agree I don't think it's a 
reason to withhold approval either.

https://github.com/llvm/llvm-project/pull/87086
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [flang] [flang][OpenMP] Move clause/object conversion to happen early, in genOMP (PR #87086)

2024-04-01 Thread Leandro Lupori via llvm-branch-commits 

https://github.com/luporl approved this pull request.

LGTM

https://github.com/llvm/llvm-project/pull/87086
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [flang] [flang][OpenMP] Move clause/object conversion to happen early, in genOMP (PR #87086)

2024-03-29 Thread via llvm-branch-commits 

llvmbot wrote:




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

Author: Krzysztof Parzyszek (kparzysz)


Changes

This removes the last use of genOmpObectList2, which has now been removed.

---

Patch is 58.36 KiB, truncated to 20.00 KiB below, full version: 
https://github.com/llvm/llvm-project/pull/87086.diff


5 Files Affected:

- (modified) flang/lib/Lower/OpenMP/ClauseProcessor.h (+2-3) 
- (modified) flang/lib/Lower/OpenMP/DataSharingProcessor.h (+2-3) 
- (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+201-223) 
- (modified) flang/lib/Lower/OpenMP/Utils.cpp (+11-19) 
- (modified) flang/lib/Lower/OpenMP/Utils.h (+2-4) 


``diff
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.h 
b/flang/lib/Lower/OpenMP/ClauseProcessor.h
index db7a1b8335f818..f4d659b70cfee7 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.h
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.h
@@ -49,9 +49,8 @@ class ClauseProcessor {
 public:
   ClauseProcessor(Fortran::lower::AbstractConverter ,
   Fortran::semantics::SemanticsContext ,
-  const Fortran::parser::OmpClauseList )
-  : converter(converter), semaCtx(semaCtx),
-clauses(makeClauses(clauses, semaCtx)) {}
+  const List )
+  : converter(converter), semaCtx(semaCtx), clauses(clauses) {}
 
   // 'Unique' clauses: They can appear at most once in the clause list.
   bool processCollapse(
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h 
b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index c11ee299c5d085..ef7b14327278e3 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -78,13 +78,12 @@ class DataSharingProcessor {
 public:
   DataSharingProcessor(Fortran::lower::AbstractConverter ,
Fortran::semantics::SemanticsContext ,
-   const Fortran::parser::OmpClauseList ,
+   const List ,
Fortran::lower::pft::Evaluation ,
bool useDelayedPrivatization = false,
Fortran::lower::SymMap *symTable = nullptr)
   : hasLastPrivateOp(false), converter(converter),
-firOpBuilder(converter.getFirOpBuilder()),
-clauses(omp::makeClauses(opClauseList, semaCtx)), eval(eval),
+firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), 
eval(eval),
 useDelayedPrivatization(useDelayedPrivatization), symTable(symTable) {}
 
   // Privatisation is split into two steps.
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp 
b/flang/lib/Lower/OpenMP/OpenMP.cpp
index edae453972d3d9..23dc25ac1ae9a1 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -17,6 +17,7 @@
 #include "DataSharingProcessor.h"
 #include "DirectivesCommon.h"
 #include "ReductionProcessor.h"
+#include "Utils.h"
 #include "flang/Common/idioms.h"
 #include "flang/Lower/Bridge.h"
 #include "flang/Lower/ConvertExpr.h"
@@ -310,14 +311,15 @@ static void getDeclareTargetInfo(
   } else if (const auto *clauseList{
  Fortran::parser::Unwrap(
  spec.u)}) {
-if (clauseList->v.empty()) {
+List clauses = makeClauses(*clauseList, semaCtx);
+if (clauses.empty()) {
   // Case: declare target, implicit capture of function
   symbolAndClause.emplace_back(
   mlir::omp::DeclareTargetCaptureClause::to,
   eval.getOwningProcedure()->getSubprogramSymbol());
 }
 
-ClauseProcessor cp(converter, semaCtx, *clauseList);
+ClauseProcessor cp(converter, semaCtx, clauses);
 cp.processDeviceType(clauseOps);
 cp.processEnter(symbolAndClause);
 cp.processLink(symbolAndClause);
@@ -597,14 +599,11 @@ static void removeStoreOp(mlir::Operation *reductionOp, 
mlir::Value symVal) {
 // TODO: Generate the reduction operation during lowering instead of creating
 // and removing operations since this is not a robust approach. Also, removing
 // ops in the builder (instead of a rewriter) is probably not the best 
approach.
-static void
-genOpenMPReduction(Fortran::lower::AbstractConverter ,
-   Fortran::semantics::SemanticsContext ,
-   const Fortran::parser::OmpClauseList ) {
+static void genOpenMPReduction(Fortran::lower::AbstractConverter ,
+   Fortran::semantics::SemanticsContext ,
+   const List ) {
   fir::FirOpBuilder  = converter.getFirOpBuilder();
 
-  List clauses{makeClauses(clauseList, semaCtx)};
-
   for (const Clause  : clauses) {
 if (const auto  =
 std::get_if()) {
@@ -812,7 +811,7 @@ struct OpWithBodyGenInfo {
 return *this;
   }
 
-  OpWithBodyGenInfo (const Fortran::parser::OmpClauseList *value) {
+  OpWithBodyGenInfo (const List *value) {
 clauses = value;
 return *this;
   }
@@ -848,7 +847,7 @@ struct OpWithBodyGenInfo {
   /// [in] is this an outer operation - prevents privatization.
   bool outerCombined = false;
   ///

[llvm-branch-commits] [flang] [flang][OpenMP] Move clause/object conversion to happen early, in genOMP (PR #87086)

2024-03-29 Thread Krzysztof Parzyszek via llvm-branch-commits 

https://github.com/kparzysz created 
https://github.com/llvm/llvm-project/pull/87086

This removes the last use of genOmpObectList2, which has now been removed.

>From f725face892cef4faf9f17d4b549541bdbcd7e08 Mon Sep 17 00:00:00 2001
From: Krzysztof Parzyszek 
Date: Fri, 29 Mar 2024 09:20:41 -0500
Subject: [PATCH] [flang][OpenMP] Move clause/object conversion to happen
 early, in genOMP

This removes the last use of genOmpObectList2, which has now been removed.
---
 flang/lib/Lower/OpenMP/ClauseProcessor.h  |   5 +-
 flang/lib/Lower/OpenMP/DataSharingProcessor.h |   5 +-
 flang/lib/Lower/OpenMP/OpenMP.cpp | 424 +-
 flang/lib/Lower/OpenMP/Utils.cpp  |  30 +-
 flang/lib/Lower/OpenMP/Utils.h|   6 +-
 5 files changed, 218 insertions(+), 252 deletions(-)

diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.h 
b/flang/lib/Lower/OpenMP/ClauseProcessor.h
index db7a1b8335f818..f4d659b70cfee7 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.h
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.h
@@ -49,9 +49,8 @@ class ClauseProcessor {
 public:
   ClauseProcessor(Fortran::lower::AbstractConverter ,
   Fortran::semantics::SemanticsContext ,
-  const Fortran::parser::OmpClauseList )
-  : converter(converter), semaCtx(semaCtx),
-clauses(makeClauses(clauses, semaCtx)) {}
+  const List )
+  : converter(converter), semaCtx(semaCtx), clauses(clauses) {}
 
   // 'Unique' clauses: They can appear at most once in the clause list.
   bool processCollapse(
diff --git a/flang/lib/Lower/OpenMP/DataSharingProcessor.h 
b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
index c11ee299c5d085..ef7b14327278e3 100644
--- a/flang/lib/Lower/OpenMP/DataSharingProcessor.h
+++ b/flang/lib/Lower/OpenMP/DataSharingProcessor.h
@@ -78,13 +78,12 @@ class DataSharingProcessor {
 public:
   DataSharingProcessor(Fortran::lower::AbstractConverter ,
Fortran::semantics::SemanticsContext ,
-   const Fortran::parser::OmpClauseList ,
+   const List ,
Fortran::lower::pft::Evaluation ,
bool useDelayedPrivatization = false,
Fortran::lower::SymMap *symTable = nullptr)
   : hasLastPrivateOp(false), converter(converter),
-firOpBuilder(converter.getFirOpBuilder()),
-clauses(omp::makeClauses(opClauseList, semaCtx)), eval(eval),
+firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), 
eval(eval),
 useDelayedPrivatization(useDelayedPrivatization), symTable(symTable) {}
 
   // Privatisation is split into two steps.
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp 
b/flang/lib/Lower/OpenMP/OpenMP.cpp
index edae453972d3d9..23dc25ac1ae9a1 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -17,6 +17,7 @@
 #include "DataSharingProcessor.h"
 #include "DirectivesCommon.h"
 #include "ReductionProcessor.h"
+#include "Utils.h"
 #include "flang/Common/idioms.h"
 #include "flang/Lower/Bridge.h"
 #include "flang/Lower/ConvertExpr.h"
@@ -310,14 +311,15 @@ static void getDeclareTargetInfo(
   } else if (const auto *clauseList{
  Fortran::parser::Unwrap(
  spec.u)}) {
-if (clauseList->v.empty()) {
+List clauses = makeClauses(*clauseList, semaCtx);
+if (clauses.empty()) {
   // Case: declare target, implicit capture of function
   symbolAndClause.emplace_back(
   mlir::omp::DeclareTargetCaptureClause::to,
   eval.getOwningProcedure()->getSubprogramSymbol());
 }
 
-ClauseProcessor cp(converter, semaCtx, *clauseList);
+ClauseProcessor cp(converter, semaCtx, clauses);
 cp.processDeviceType(clauseOps);
 cp.processEnter(symbolAndClause);
 cp.processLink(symbolAndClause);
@@ -597,14 +599,11 @@ static void removeStoreOp(mlir::Operation *reductionOp, 
mlir::Value symVal) {
 // TODO: Generate the reduction operation during lowering instead of creating
 // and removing operations since this is not a robust approach. Also, removing
 // ops in the builder (instead of a rewriter) is probably not the best 
approach.
-static void
-genOpenMPReduction(Fortran::lower::AbstractConverter ,
-   Fortran::semantics::SemanticsContext ,
-   const Fortran::parser::OmpClauseList ) {
+static void genOpenMPReduction(Fortran::lower::AbstractConverter ,
+   Fortran::semantics::SemanticsContext ,
+   const List ) {
   fir::FirOpBuilder  = converter.getFirOpBuilder();
 
-  List clauses{makeClauses(clauseList, semaCtx)};
-
   for (const Clause  : clauses) {
 if (const auto  =
 std::get_if()) {
@@ -812,7 +811,7 @@ struct OpWithBodyGenInfo {
 return *this;
   }
 
-  OpWithBodyGenInfo (const Fortran::parser::OmpClauseList *value) {
+  OpWithBodyGenInfo (const List *value) {
 clauses =