[flang-commits] [flang] [flang][OpenMP][HLFIR] Support vector subscripted array sections for DEPEND (PR #133892)
Tom Eccles via flang-commits
flang-commits at lists.llvm.org
Mon Apr 14 03:07:40 PDT 2025
https://github.com/tblah updated https://github.com/llvm/llvm-project/pull/133892
>From e748dc55e1a42bca2a4c81413bd146342d781bab Mon Sep 17 00:00:00 2001
From: Tom Eccles <tom.eccles at arm.com>
Date: Thu, 27 Mar 2025 11:49:08 +0000
Subject: [PATCH 1/2] [flang][OpenMP][HLFIR] Support vector subscripted array
sections for DEPEND
The OpenMP runtime needs the base address of the array section to
identify the dependency.
If we just put the vector subscript through the usual HLFIR expression
lowering, that would generate a new contiguous array representing the
values of the elements in the array which was sectioned. We cannot use
addresses from this array because these addresses would not match
dependencies on the original array. For example
```
integer :: array(1024)
integer :: indices(2)
indices(1) = 1
indices(2) = 100
!$omp task depend(out: array(1:512))
!$omp end task
!$omp task depend(in: array(indices))
!$omp end task
```
This requires taking the lowering path previously only used for ordered
assignments to get the address of the elements in the original array
which were indexed. This is done using `hlfir.elemental_addr`. e.g.
```
array(indices) = 2
```
`hlfir.elemental_addr` is awkward to use because it (by design) doesn't
return something like `!hlfir.expr<>` (like `hlfir.elemental`) and so
it can't have a generic lowering: each place it is used has to
carefully inline the contents of the operation and extract the needed
address.
For this reason, `hlfir.elemental_addr` was not previously allowed
outside of these ordered assignments. In this commit I relax that
restriction so that I can use `hlfir.elemental_addr` to lower the OpenMP
DEPEND clause. The disadvantage of doing so is that there is no generic
lowering supporting uses of `hlfir.elemental_addr` in arbitrary
contexts. I think it is okay to have this gap in the dialect verifier
because HLFIR is not a dialect shared with users outside of flang.
One alternative solution would have been to provide my own more limited
re-implementation of `HlfirDesignatorBuilder` which skipped
`hlfir::elemental_addr`, instead inlining its body directly at the
current insertion point applying indices only for the first element. This
would have been difficult to maintain because designation in Fortran is
complex.
---
.../include/flang/Optimizer/HLFIR/HLFIROps.td | 12 ++---
flang/lib/Lower/OpenMP/ClauseProcessor.cpp | 54 ++++++++++++++++---
...-clause-vector-subscript-array-section.f90 | 11 ----
.../OpenMP/task-depend-array-section.f90 | 33 ++++++++++++
4 files changed, 86 insertions(+), 24 deletions(-)
delete mode 100644 flang/test/Lower/OpenMP/Todo/depend-clause-vector-subscript-array-section.f90
diff --git a/flang/include/flang/Optimizer/HLFIR/HLFIROps.td b/flang/include/flang/Optimizer/HLFIR/HLFIROps.td
index f69930d5b53b3..cf2c78d2a12b2 100644
--- a/flang/include/flang/Optimizer/HLFIR/HLFIROps.td
+++ b/flang/include/flang/Optimizer/HLFIR/HLFIROps.td
@@ -1419,28 +1419,26 @@ def hlfir_YieldOp : hlfir_Op<"yield", [Terminator, ParentOneOf<["RegionAssignOp"
let assemblyFormat = "$entity attr-dict `:` type($entity) custom<YieldOpCleanup>($cleanup)";
}
-def hlfir_ElementalAddrOp : hlfir_Op<"elemental_addr", [Terminator, HasParent<"RegionAssignOp">,
+def hlfir_ElementalAddrOp : hlfir_Op<"elemental_addr", [Terminator,
RecursiveMemoryEffects, RecursivelySpeculatable, hlfir_ElementalOpInterface,
AttrSizedOperandSegments]> {
let summary = "Yield the address of a vector subscripted variable inside an hlfir.region_assign";
let description = [{
- Special terminator node for the left-hand side region of an hlfir.region_assign
- to a vector subscripted entity.
+ Used to get the address of elements in an array which is being subscripted
+ by a vector.
It represents how the address of an element of such entity is computed given
one based indices.
It is very similar to hlfir.elemental, except that it does not produce an SSA
value because there is no hlfir type to describe a vector subscripted entity
- (the codegen of such type would be problematic). Hence, it is tightly linked
- to an hlfir.region_assign by its terminator property.
+ (the codegen of such type would be problematic).
An optional cleanup region may be provided if any of the subscript expressions
of the designator require a cleanup.
This allows documenting cleanups that cannot be generated after the vector
subscripted designator usage (that has not been materizaled yet). The cleanups
- will be evaluated after the assignment once the related
- hlfir.region_assign is lowered.
+ should be evaluated after the usage of the elemental addr.
Example: "X(VECTOR) = Y"
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 44796994b244c..9432fe06b00b7 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -889,13 +889,55 @@ bool ClauseProcessor::processDepend(lower::SymMap &symMap,
} else if (evaluate::IsArrayElement(*object.ref())) {
// Array Section
SomeExpr expr = *object.ref();
- if (isVectorSubscript(expr))
- TODO(converter.getCurrentLocation(),
- "Vector subscripted array section for task dependency");
- hlfir::EntityWithAttributes entity = convertExprToHLFIR(
- converter.getCurrentLocation(), converter, expr, symMap, stmtCtx);
- dependVar = entity.getBase();
+ if (isVectorSubscript(expr)) {
+ // OpenMP needs the address of the first indexed element (required by
+ // the standard to be the lowest index) to identify the dependency. We
+ // don't need an accurate length for the array section because the
+ // OpenMP standard forbids overlapping array sections.
+
+ // Get a hlfir.elemental_addr op describing the address of the value
+ // indexed from the original array.
+ hlfir::ElementalAddrOp addrOp =
+ convertVectorSubscriptedExprToElementalAddr(
+ converter.getCurrentLocation(), converter, expr, symMap,
+ stmtCtx);
+ if (!addrOp.getCleanup().empty())
+ TODO(converter.getCurrentLocation(),
+ "Vector subscript in DEPEND clause requring a cleanup region");
+
+ // hlfir.elemental_addr doesn't have a normal lowering because it
+ // can't return a value. Instead we need to inline it here using
+ // values for the first element. Similar to hlfir::inlineElementalOp.
+
+ mlir::Value one = builder.createIntegerConstant(
+ converter.getCurrentLocation(), builder.getIndexType(), 1);
+ mlir::SmallVector<mlir::Value> oneBasedIndices;
+ oneBasedIndices.resize(addrOp.getIndices().size(), one);
+
+ mlir::IRMapping mapper;
+ mapper.map(addrOp.getIndices(), oneBasedIndices);
+ assert(addrOp.getElementalRegion().hasOneBlock());
+ mlir::Operation *newOp;
+ for (mlir::Operation &op :
+ addrOp.getElementalRegion().back().getOperations())
+ newOp = builder.clone(op, mapper);
+ auto yield = mlir::cast<hlfir::YieldOp>(newOp);
+
+ addrOp->erase();
+
+ if (!yield.getCleanup().empty())
+ TODO(converter.getCurrentLocation(),
+ "Vector subscript in DEPEND clause requring element cleanup");
+
+ dependVar = yield.getEntity();
+ yield->erase();
+ } else {
+ // Ordinary array section e.g. A(1:512:2)
+ hlfir::EntityWithAttributes entity = convertExprToHLFIR(
+ converter.getCurrentLocation(), converter, expr, symMap, stmtCtx);
+ dependVar = entity.getBase();
+ }
} else {
semantics::Symbol *sym = object.sym();
dependVar = converter.getSymbolAddress(*sym);
diff --git a/flang/test/Lower/OpenMP/Todo/depend-clause-vector-subscript-array-section.f90 b/flang/test/Lower/OpenMP/Todo/depend-clause-vector-subscript-array-section.f90
deleted file mode 100644
index f3bd58c8c559a..0000000000000
--- a/flang/test/Lower/OpenMP/Todo/depend-clause-vector-subscript-array-section.f90
+++ /dev/null
@@ -1,11 +0,0 @@
-! RUN: %not_todo_cmd bbc -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s
-! RUN: %not_todo_cmd %flang_fc1 -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s
-
-! CHECK: Vector subscripted array section for task dependency
-subroutine vectorSubscriptArraySection(array, indices)
- integer :: array(:)
- integer :: indices(:)
-
- !$omp task depend (in: array(indices))
- !$omp end task
-end subroutine
diff --git a/flang/test/Lower/OpenMP/task-depend-array-section.f90 b/flang/test/Lower/OpenMP/task-depend-array-section.f90
index b364a5e06a29c..4033b8ed1abf3 100644
--- a/flang/test/Lower/OpenMP/task-depend-array-section.f90
+++ b/flang/test/Lower/OpenMP/task-depend-array-section.f90
@@ -49,3 +49,36 @@ subroutine assumedShape(array)
! CHECK: }
! CHECK: return
! CHECK: }
+
+subroutine vectorSubscriptArraySection(array, indices)
+ integer :: array(:)
+ integer :: indices(:)
+
+ !$omp task depend (in: array(indices))
+ !$omp end task
+end subroutine
+! CHECK-LABEL: func.func @_QPvectorsubscriptarraysection(
+! CHECK-SAME: %[[VAL_0:[0-9]+|[a-zA-Z$._-][a-zA-Z0-9$._-]*]]: !fir.box<!fir.array<?xi32>> {fir.bindc_name = "array"},
+! CHECK-SAME: %[[VAL_1:[0-9]+|[a-zA-Z$._-][a-zA-Z0-9$._-]*]]: !fir.box<!fir.array<?xi32>> {fir.bindc_name = "indices"}) {
+! CHECK: %[[VAL_2:.*]] = fir.dummy_scope : !fir.dscope
+! CHECK: %[[VAL_3:.*]]:2 = hlfir.declare %[[VAL_0]] dummy_scope %[[VAL_2]] {uniq_name = "_QFvectorsubscriptarraysectionEarray"} : (!fir.box<!fir.array<?xi32>>, !fir.dscope) -> (!fir.box<!fir.array<?xi32>>, !fir.box<!fir.array<?xi32>>)
+! CHECK: %[[VAL_4:.*]]:2 = hlfir.declare %[[VAL_1]] dummy_scope %[[VAL_2]] {uniq_name = "_QFvectorsubscriptarraysectionEindices"} : (!fir.box<!fir.array<?xi32>>, !fir.dscope) -> (!fir.box<!fir.array<?xi32>>, !fir.box<!fir.array<?xi32>>)
+! CHECK: %[[VAL_5:.*]] = arith.constant 0 : index
+! CHECK: %[[VAL_6:.*]]:3 = fir.box_dims %[[VAL_4]]#0, %[[VAL_5]] : (!fir.box<!fir.array<?xi32>>, index) -> (index, index, index)
+! CHECK: %[[VAL_7:.*]] = fir.shape %[[VAL_6]]#1 : (index) -> !fir.shape<1>
+! CHECK: %[[VAL_8:.*]] = hlfir.elemental %[[VAL_7]] unordered : (!fir.shape<1>) -> !hlfir.expr<?xi64> {
+! CHECK: ^bb0(%[[VAL_9:.*]]: index):
+! CHECK: %[[VAL_10:.*]] = hlfir.designate %[[VAL_4]]#0 (%[[VAL_9]]) : (!fir.box<!fir.array<?xi32>>, index) -> !fir.ref<i32>
+! CHECK: %[[VAL_11:.*]] = fir.load %[[VAL_10]] : !fir.ref<i32>
+! CHECK: %[[VAL_12:.*]] = fir.convert %[[VAL_11]] : (i32) -> i64
+! CHECK: hlfir.yield_element %[[VAL_12]] : i64
+! CHECK: }
+! CHECK: %[[VAL_13:.*]] = arith.constant 1 : index
+! CHECK: %[[VAL_14:.*]] = hlfir.apply %[[VAL_8]], %[[VAL_13]] : (!hlfir.expr<?xi64>, index) -> i64
+! CHECK: %[[VAL_15:.*]] = hlfir.designate %[[VAL_3]]#0 (%[[VAL_14]]) : (!fir.box<!fir.array<?xi32>>, i64) -> !fir.ref<i32>
+! CHECK: omp.task depend(taskdependin -> %[[VAL_15]] : !fir.ref<i32>) {
+! CHECK: omp.terminator
+! CHECK: }
+! CHECK: hlfir.destroy %[[VAL_8]] : !hlfir.expr<?xi64>
+! CHECK: return
+! CHECK: }
>From 358194416a76ecb31fbc1c1973220dd605e8edca Mon Sep 17 00:00:00 2001
From: Tom Eccles <tom.eccles at arm.com>
Date: Mon, 14 Apr 2025 10:01:42 +0000
Subject: [PATCH 2/2] Revert HLFIR op change
---
flang/include/flang/Optimizer/HLFIR/HLFIROps.td | 12 +++++++-----
flang/lib/Lower/OpenMP/ClauseProcessor.cpp | 7 +++++++
2 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/flang/include/flang/Optimizer/HLFIR/HLFIROps.td b/flang/include/flang/Optimizer/HLFIR/HLFIROps.td
index cf2c78d2a12b2..f69930d5b53b3 100644
--- a/flang/include/flang/Optimizer/HLFIR/HLFIROps.td
+++ b/flang/include/flang/Optimizer/HLFIR/HLFIROps.td
@@ -1419,26 +1419,28 @@ def hlfir_YieldOp : hlfir_Op<"yield", [Terminator, ParentOneOf<["RegionAssignOp"
let assemblyFormat = "$entity attr-dict `:` type($entity) custom<YieldOpCleanup>($cleanup)";
}
-def hlfir_ElementalAddrOp : hlfir_Op<"elemental_addr", [Terminator,
+def hlfir_ElementalAddrOp : hlfir_Op<"elemental_addr", [Terminator, HasParent<"RegionAssignOp">,
RecursiveMemoryEffects, RecursivelySpeculatable, hlfir_ElementalOpInterface,
AttrSizedOperandSegments]> {
let summary = "Yield the address of a vector subscripted variable inside an hlfir.region_assign";
let description = [{
- Used to get the address of elements in an array which is being subscripted
- by a vector.
+ Special terminator node for the left-hand side region of an hlfir.region_assign
+ to a vector subscripted entity.
It represents how the address of an element of such entity is computed given
one based indices.
It is very similar to hlfir.elemental, except that it does not produce an SSA
value because there is no hlfir type to describe a vector subscripted entity
- (the codegen of such type would be problematic).
+ (the codegen of such type would be problematic). Hence, it is tightly linked
+ to an hlfir.region_assign by its terminator property.
An optional cleanup region may be provided if any of the subscript expressions
of the designator require a cleanup.
This allows documenting cleanups that cannot be generated after the vector
subscripted designator usage (that has not been materizaled yet). The cleanups
- should be evaluated after the usage of the elemental addr.
+ will be evaluated after the assignment once the related
+ hlfir.region_assign is lowered.
Example: "X(VECTOR) = Y"
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index 9432fe06b00b7..cce6dc32bad4b 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -898,6 +898,13 @@ bool ClauseProcessor::processDepend(lower::SymMap &symMap,
// Get a hlfir.elemental_addr op describing the address of the value
// indexed from the original array.
+ // Note: the hlfir.elemental_addr op verifier requires it to be inside
+ // of a hlfir.region_assign op. This is because the only place in base
+ // Fortran where you need the address of a vector subscript would be
+ // in an assignment operation. We are not doing an assignment here
+ // but we do want the address (without having to duplicate all of
+ // Fortran designation lowering!). This operation is never seen by the
+ // verifier because it is immediately inlined.
hlfir::ElementalAddrOp addrOp =
convertVectorSubscriptedExprToElementalAddr(
converter.getCurrentLocation(), converter, expr, symMap,
More information about the flang-commits
mailing list