Skip to content

Conversation

@ergawy
Copy link
Member

@ergawy ergawy commented Jun 27, 2025

@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Jun 27, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 27, 2025

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

Author: Kareem Ergawy (ergawy)

Changes

Re-organizes the op definition a little bit and removes a method that does not add much value to the API.


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

2 Files Affected:

  • (modified) flang/include/flang/Optimizer/Dialect/FIROps.td (+10-12)
  • (modified) flang/lib/Optimizer/OpenMP/DoConcurrentConversion.cpp (+2-2)
diff --git a/flang/include/flang/Optimizer/Dialect/FIROps.td b/flang/include/flang/Optimizer/Dialect/FIROps.td
index 75cf020ec96c8..f304f0cf30ac5 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROps.td
+++ b/flang/include/flang/Optimizer/Dialect/FIROps.td
@@ -3884,9 +3884,17 @@ def fir_DoConcurrentLoopOp : fir_Op<"do_concurrent.loop",
   let hasVerifier = 1;
 
   let extraClassDeclaration = [{
-    unsigned getNumInductionVars() { return getLowerBound().size(); }
+    unsigned getNumInductionVars() {
+      return getLowerBound().size();
+    }
 
-    unsigned getNumLocalOperands() { return getLocalVars().size(); }
+    unsigned getNumLocalOperands() {
+      return getLocalVars().size();
+    }
+
+    unsigned getNumReduceOperands() {
+      return getReduceVars().size();
+    }
 
     mlir::Block::BlockArgListType getInductionVars() {
       return getBody()->getArguments().slice(0, getNumInductionVars());
@@ -3906,16 +3914,6 @@ def fir_DoConcurrentLoopOp : fir_Op<"do_concurrent.loop",
     /// Number of operands controlling the loop
     unsigned getNumControlOperands() { return getLowerBound().size() * 3; }
 
-    // Get Number of reduction operands
-    unsigned getNumReduceOperands() {
-      return getReduceVars().size();
-    }
-
-    mlir::Operation::operand_range getLocalOperands() {
-      return getOperands()
-          .slice(getNumControlOperands() + getNumReduceOperands(),
-                 getNumLocalOperands());
-    }
   }];
 }
 
diff --git a/flang/lib/Optimizer/OpenMP/DoConcurrentConversion.cpp b/flang/lib/Optimizer/OpenMP/DoConcurrentConversion.cpp
index 28f6c8bf02813..709cf1d0938fa 100644
--- a/flang/lib/Optimizer/OpenMP/DoConcurrentConversion.cpp
+++ b/flang/lib/Optimizer/OpenMP/DoConcurrentConversion.cpp
@@ -314,9 +314,9 @@ class DoConcurrentConversion
 
     // For `local` (and `local_init`) opernads, emit corresponding `private`
     // clauses and attach these clauses to the workshare loop.
-    if (!loop.getLocalOperands().empty())
+    if (!loop.getLocalVars().empty())
       for (auto [op, sym, arg] : llvm::zip_equal(
-               loop.getLocalOperands(),
+               loop.getLocalVars(),
                loop.getLocalSymsAttr().getAsRange<mlir::SymbolRefAttr>(),
                loop.getRegionLocalArgs())) {
         auto localizer = mlir::SymbolTable::lookupNearestSymbolFrom<

@ergawy ergawy force-pushed the users/ergawy/convert_locality_specs_to_clauses_6 branch from 5073e7d to 9021dbc Compare June 27, 2025 07:01
@ergawy ergawy force-pushed the users/ergawy/convert_locality_specs_to_clauses_7 branch from d9ca670 to 5f665c9 Compare June 27, 2025 07:01
Copy link
Member

@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.

LGTM, thanks!

@ergawy ergawy force-pushed the users/ergawy/convert_locality_specs_to_clauses_6 branch from 9021dbc to 6a7c71e Compare June 30, 2025 04:18
@ergawy ergawy force-pushed the users/ergawy/convert_locality_specs_to_clauses_7 branch 2 times, most recently from 105ac7a to 3e2bdf5 Compare June 30, 2025 04:40
@ergawy ergawy force-pushed the users/ergawy/convert_locality_specs_to_clauses_6 branch from 926cb7c to aa3984a Compare July 2, 2025 15:41
@ergawy ergawy force-pushed the users/ergawy/convert_locality_specs_to_clauses_7 branch from 3e2bdf5 to 15ed831 Compare July 2, 2025 15:42
@ergawy ergawy force-pushed the users/ergawy/convert_locality_specs_to_clauses_6 branch from aa3984a to 4db4b85 Compare July 9, 2025 07:18
@ergawy ergawy force-pushed the users/ergawy/convert_locality_specs_to_clauses_7 branch from 15ed831 to 53f9c0d Compare July 9, 2025 07:18
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Jul 10, 2025
…lled in OpenMP and OpenACC

This PR proposes re-modelling `reduce` specifiers to match OpenMP and OpenACC. In particular, this PR includes the following:

* A new `fir` op: `fir.delcare_reduction` which is identical to OpenMP's `omp.declare_reduction` op.
* Updating the `reduce` clause on `fir.do_concurrent.loop` to use the new op.
* Re-uses the `ReductionProcessor` component to emit reductions for `do concurrent` just like we do for OpenMP. To do this, the `ReductionProcessor` had to be refactored to be more generalized.
* Upates mapping `do concurrent` to `fir.loop ... unordered` nests using the new reduction model.

Unfortunately, this is a big PR that would be difficult to divide up in smaller parts because the bottom of the changes are the `fir` table-gen changes to `do concurrent`. However, doing these MLIR changes cascades to the other parts that have to be modified to not break things.

This PR goes in the same direction we went for `private/local` speicifiers. Now the `do concurrent` and OpenMP (and OpenACC) dialects are modelled in essentially the same way which makes mapping between them more trivial, hopefully.

PR stack:
- llvm#145837 (this one)
- llvm#146025
- llvm#146028
- llvm#146033
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Jul 10, 2025
With llvm#145837, the `ReductionProcessor` component is now used by both OpenMP and `do concurrent`. Therefore, this PR moves it to a shared location: `flang/Lower/Support`.

PR stack:
- llvm#145837
- llvm#146025 (this one)
- llvm#146028
- llvm#146033
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Jul 10, 2025
Re-organizes the op definition a little bit and removes a method that does not add much value to the API.

PR stack:
- llvm#145837
- llvm#146025
- llvm#146028 (this one)
- llvm#146033
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Jul 10, 2025
Now that we have changes introduced by llvm#145837, mapping reductions from `do concurrent` to OpenMP is almost trivial. This PR adds such mapping.

PR stack:
- llvm#145837
- llvm#146025
- llvm#146028
- llvm#146033 (this one)
ergawy added a commit that referenced this pull request Jul 11, 2025
…lled in OpenMP and OpenACC (#145837)

This PR proposes re-modelling `reduce` specifiers to match OpenMP and
OpenACC. In particular, this PR includes the following:

* A new `fir` op: `fir.delcare_reduction` which is identical to OpenMP's
`omp.declare_reduction` op.
* Updating the `reduce` clause on `fir.do_concurrent.loop` to use the
new op.
* Re-uses the `ReductionProcessor` component to emit reductions for `do
concurrent` just like we do for OpenMP. To do this, the
`ReductionProcessor` had to be refactored to be more generalized.
* Upates mapping `do concurrent` to `fir.loop ... unordered` nests using
the new reduction model.

Unfortunately, this is a big PR that would be difficult to divide up in
smaller parts because the bottom of the changes are the `fir` table-gen
changes to `do concurrent`. However, doing these MLIR changes cascades
to the other parts that have to be modified to not break things.

This PR goes in the same direction we went for `private/local`
speicifiers. Now the `do concurrent` and OpenMP (and OpenACC) dialects
are modelled in essentially the same way which makes mapping between
them more trivial, hopefully.

PR stack:
- #145837 (this one)
- #146025
- #146028
- #146033
@ergawy ergawy force-pushed the users/ergawy/convert_locality_specs_to_clauses_6 branch from 4db4b85 to e9eb77f Compare July 11, 2025 04:40
@ergawy ergawy force-pushed the users/ergawy/convert_locality_specs_to_clauses_7 branch from 53f9c0d to 8e67de7 Compare July 11, 2025 04:41
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Jul 11, 2025
…ns are modelled in OpenMP and OpenACC (#145837)

This PR proposes re-modelling `reduce` specifiers to match OpenMP and
OpenACC. In particular, this PR includes the following:

* A new `fir` op: `fir.delcare_reduction` which is identical to OpenMP's
`omp.declare_reduction` op.
* Updating the `reduce` clause on `fir.do_concurrent.loop` to use the
new op.
* Re-uses the `ReductionProcessor` component to emit reductions for `do
concurrent` just like we do for OpenMP. To do this, the
`ReductionProcessor` had to be refactored to be more generalized.
* Upates mapping `do concurrent` to `fir.loop ... unordered` nests using
the new reduction model.

Unfortunately, this is a big PR that would be difficult to divide up in
smaller parts because the bottom of the changes are the `fir` table-gen
changes to `do concurrent`. However, doing these MLIR changes cascades
to the other parts that have to be modified to not break things.

This PR goes in the same direction we went for `private/local`
speicifiers. Now the `do concurrent` and OpenMP (and OpenACC) dialects
are modelled in essentially the same way which makes mapping between
them more trivial, hopefully.

PR stack:
- llvm/llvm-project#145837 (this one)
- llvm/llvm-project#146025
- llvm/llvm-project#146028
- llvm/llvm-project#146033
ergawy added a commit that referenced this pull request Jul 11, 2025
With #145837, the `ReductionProcessor` component is now used by both
OpenMP and `do concurrent`. Therefore, this PR moves it to a shared
location: `flang/Lower/Support`.

PR stack:
- #145837
- #146025 (this one)
- #146028
- #146033
Base automatically changed from users/ergawy/convert_locality_specs_to_clauses_6 to main July 11, 2025 05:42
@ergawy ergawy force-pushed the users/ergawy/convert_locality_specs_to_clauses_7 branch from 8206c40 to 8e67de7 Compare July 11, 2025 05:44
Re-organizes the op definition a little bit and removes a method that
does not add much value to the API.
@ergawy ergawy force-pushed the users/ergawy/convert_locality_specs_to_clauses_7 branch from 8e67de7 to df63fea Compare July 11, 2025 05:45
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Jul 11, 2025
… (#146025)

With #145837, the `ReductionProcessor` component is now used by both
OpenMP and `do concurrent`. Therefore, this PR moves it to a shared
location: `flang/Lower/Support`.

PR stack:
- llvm/llvm-project#145837
- llvm/llvm-project#146025 (this one)
- llvm/llvm-project#146028
- llvm/llvm-project#146033
@ergawy ergawy merged commit a510e75 into main Jul 11, 2025
10 checks passed
@ergawy ergawy deleted the users/ergawy/convert_locality_specs_to_clauses_7 branch July 11, 2025 06:30
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Jul 11, 2025
…defintion (#146028)

Re-organizes the op definition a little bit and removes a method that
does not add much value to the API.

PR stack:
- llvm/llvm-project#145837
- llvm/llvm-project#146025
- llvm/llvm-project#146028 (this one)
- llvm/llvm-project#146033
ergawy added a commit that referenced this pull request Jul 11, 2025
…#146033)

Now that we have changes introduced by #145837, mapping reductions from
`do concurrent` to OpenMP is almost trivial. This PR adds such mapping.

PR stack:
- #145837
- #146025
- #146028
- #146033 (this one)
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Jul 11, 2025
…` to OpenMP (#146033)

Now that we have changes introduced by #145837, mapping reductions from
`do concurrent` to OpenMP is almost trivial. This PR adds such mapping.

PR stack:
- llvm/llvm-project#145837
- llvm/llvm-project#146025
- llvm/llvm-project#146028
- llvm/llvm-project#146033 (this one)
@rscottmanley
Copy link
Contributor

Please include @khaki3 on future do concurrent work, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flang:fir-hlfir flang Flang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants