[flang][Lower][OpenMP] Fix reduction on array sections aborting in lowering - #209701
Conversation
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-flang-fir-hlfir Author: jay0x (blazie2004) ChangesSummary This regression was introduced by #196094, which added a special lowering path for reductions on a single array element, such as The problem is that Flang also treated an array section like That path produced an array type that the reduction initialization code could not handle, so Flang reached a Fix The fix is to use the special element path only when the expression has rank 0, which means it represents one single value. Array sections have rank greater than 0, so they should continue through the existing boxed-array path. Fixes : 209462 Full diff: https://github.com/llvm/llvm-project/pull/209701.diff 2 Files Affected:
diff --git a/flang/lib/Lower/Support/ReductionProcessor.cpp b/flang/lib/Lower/Support/ReductionProcessor.cpp
index b0a2fe63ef81f..80d34285101f1 100644
--- a/flang/lib/Lower/Support/ReductionProcessor.cpp
+++ b/flang/lib/Lower/Support/ReductionProcessor.cpp
@@ -387,7 +387,11 @@ bool ReductionProcessor::isExpressionLoweredAsReductionObject(
if (!object || !object->ref())
return false;
const SomeExpr &expr = *object->ref();
- return evaluate::IsArrayElement(expr);
+ // Only genuine single array elements (rank 0) are lowered via the element
+ // path. Array sections such as a(2:96) and vector subscripts have rank > 0;
+ // lowering them here produces an unsupported sequence type and aborts in
+ // PrivateReductionUtils. Let them fall back to the boxed whole-array path.
+ return evaluate::IsArrayElement(expr) && expr.Rank() == 0;
}
template <typename ParentDeclOpType>
diff --git a/flang/test/Lower/OpenMP/reduction-array-section.f90 b/flang/test/Lower/OpenMP/reduction-array-section.f90
new file mode 100644
index 0000000000000..65e8b2c8fa018
--- /dev/null
+++ b/flang/test/Lower/OpenMP/reduction-array-section.f90
@@ -0,0 +1,39 @@
+! Regression test for reductions on array *sections* (e.g. a(2:96)).
+!
+! An array section has rank > 0, so it must be lowered using the boxed
+! whole-array reduction (@add_reduction_byref_box_*) and the section applied
+! via hlfir.designate inside the region. It must NOT be routed through the
+! single-element path (uniq_name = "omp.reduction.element"), which only supports
+! rank-0 references and otherwise aborts in PrivateReductionUtils with
+! "creating reduction/privatization init region for unsupported type".
+
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %s -o - | FileCheck %s --implicit-check-not=omp.reduction.element
+
+subroutine reduction_array_section(a, n)
+ integer :: a(100), n
+!$omp parallel do reduction(+: a(2:96))
+ do i = 1, n
+ a(2:96) = a(2:96) + i
+ end do
+end subroutine
+
+! CHECK: omp.declare_reduction @[[RED:add_reduction_byref_box_100xi32]] : !fir.ref<!fir.box<!fir.array<100xi32>>>
+
+! CHECK-LABEL: func.func @_QPreduction_array_section
+! CHECK: omp.wsloop {{.*}} reduction(byref @[[RED]] %{{[0-9]+}} -> %[[ARG:.*]] : !fir.ref<!fir.box<!fir.array<100xi32>>>) {
+! CHECK: %[[DECL:.*]]:2 = hlfir.declare %[[ARG]] {uniq_name = "_QFreduction_array_sectionEa"}
+! CHECK: %[[BOX:.*]] = fir.load %[[DECL]]#0 : !fir.ref<!fir.box<!fir.array<100xi32>>>
+! CHECK: hlfir.designate %[[BOX]] (%c2:%c96:%c1) {{.*}} -> !fir.ref<!fir.array<95xi32>>
+
+subroutine reduction_array_section_simd(a, n)
+ integer :: a(100), n
+!$omp parallel do simd reduction(+: a(2:96))
+ do i = 1, n
+ a(2:96) = a(2:96) + i
+ end do
+end subroutine
+
+! CHECK-LABEL: func.func @_QPreduction_array_section_simd
+! CHECK: omp.wsloop reduction(byref @[[RED]] %{{[0-9]+}} -> %[[WSARG:.*]] : !fir.ref<!fir.box<!fir.array<100xi32>>>) {
+! CHECK: omp.simd {{.*}} reduction(byref @[[RED]] %[[WSARG]] -> %[[SIMDARG:.*]] : !fir.ref<!fir.box<!fir.array<100xi32>>>) {
+! CHECK: hlfir.declare %[[SIMDARG]] {uniq_name = "_QFreduction_array_section_simdEa"}
|
There was a problem hiding this comment.
Change LGTM, just some nits that will need fixing before merging. No need for re-review from me once this is fixed.
Just to check, does this need a Assisted-by: notation in the description for AI? https://llvm.org/docs/AIToolPolicy.html
Thanks for catching this, the regression did not flag in any tests when I created the original patch.
| ! rank-0 references and otherwise aborts in PrivateReductionUtils with | ||
| ! "creating reduction/privatization init region for unsupported type". | ||
|
|
||
| ! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %s -o - | FileCheck %s --implicit-check-not=omp.reduction.element |
There was a problem hiding this comment.
nit: Usual practise is to place the RUN line at the top of the test file.
611d671 to
f91e251
Compare
|
Please make sure you use a commit message which uses the normal format for other commits in flang. In this case something like |
|
/cherry-pick 6c560cd |
|
/pull-request #211237 |
…wering (llvm#209701) **Summary** This regression was introduced by llvm#196094, which added a special lowering path for reductions on a single array element, such as `a(2)`. The problem is that Flang also treated an array section like `a(2:96)` as if it were a single element. Because of this, the section was sent to a code path that only supports scalar elements. That path produced an array type that the reduction initialization code could not handle, so Flang reached a `TODO` and aborted with a “not yet implemented” error. **Fix** The fix is to use the special element path only when the expression has rank 0, which means it represents one single value. Array sections have rank greater than 0, so they should continue through the existing boxed-array path. Fixes : [209462](llvm#209462) --------- Co-authored-by: Jay Satish Kumar Patel <kumarpat@pe31.hpc.amslabs.hpecorp.net> (cherry picked from commit 6c560cd)
…wering (llvm#209701) **Summary** This regression was introduced by llvm#196094, which added a special lowering path for reductions on a single array element, such as `a(2)`. The problem is that Flang also treated an array section like `a(2:96)` as if it were a single element. Because of this, the section was sent to a code path that only supports scalar elements. That path produced an array type that the reduction initialization code could not handle, so Flang reached a `TODO` and aborted with a “not yet implemented” error. **Fix** The fix is to use the special element path only when the expression has rank 0, which means it represents one single value. Array sections have rank greater than 0, so they should continue through the existing boxed-array path. Fixes : [209462](llvm#209462) --------- Co-authored-by: Jay Satish Kumar Patel <kumarpat@pe31.hpc.amslabs.hpecorp.net>
Revert #196094 and its follow-up #209701. The expression override mechanism does not preserve reduction-object identity across all data environments, leading to incorrect lowering for procedure-local and nested private arrays. Restore the pre-#196094 lowering while retaining semantic-context plumbing needed by later user-defined reduction support. Keep coverage showing that array-element constructs compile through the boxed-array reduction path. This was in response to this comment: #196094 (comment) Fixing exactly the bug in the comment wasn't hard but AI code review found a large number of follow on bugs so I think the design needs a rethink, and definitely shouldn't be included in the LLVM release. Assisted-by: Codex
Revert llvm#196094 and its follow-up llvm#209701. The expression override mechanism does not preserve reduction-object identity across all data environments, leading to incorrect lowering for procedure-local and nested private arrays. Restore the pre-llvm#196094 lowering while retaining semantic-context plumbing needed by later user-defined reduction support. Keep coverage showing that array-element constructs compile through the boxed-array reduction path. This was in response to this comment: llvm#196094 (comment) Fixing exactly the bug in the comment wasn't hard but AI code review found a large number of follow on bugs so I think the design needs a rethink, and definitely shouldn't be included in the LLVM release. Assisted-by: Codex
Revert llvm#196094 and its follow-up llvm#209701. The expression override mechanism does not preserve reduction-object identity across all data environments, leading to incorrect lowering for procedure-local and nested private arrays. Restore the pre-llvm#196094 lowering while retaining semantic-context plumbing needed by later user-defined reduction support. Keep coverage showing that array-element constructs compile through the boxed-array reduction path. This was in response to this comment: llvm#196094 (comment) Fixing exactly the bug in the comment wasn't hard but AI code review found a large number of follow on bugs so I think the design needs a rethink, and definitely shouldn't be included in the LLVM release. Assisted-by: Codex
Revert llvm#196094 and its follow-up llvm#209701. The expression override mechanism does not preserve reduction-object identity across all data environments, leading to incorrect lowering for procedure-local and nested private arrays. Restore the pre-llvm#196094 lowering. Keep coverage showing that array-element constructs compile through the boxed-array reduction path. This was in response to this comment: llvm#196094 (comment) Fixing exactly the bug in the comment wasn't hard but AI code review found a large number of follow on bugs so I think the design needs a rethink, and definitely shouldn't be included in the LLVM release. Assisted-by: Codex
Summary
This regression was introduced by #196094, which added a special lowering path for reductions on a single array element, such as
a(2).The problem is that Flang also treated an array section like
a(2:96)as if it were a single element. Because of this, the section was sent to a code path that only supports scalar elements.That path produced an array type that the reduction initialization code could not handle, so Flang reached a
TODOand aborted with a “not yet implemented” error.Fix
The fix is to use the special element path only when the expression has rank 0, which means it represents one single value. Array sections have rank greater than 0, so they should continue through the existing boxed-array path.
Fixes : 209462