[Flang][Alias analysis] Fix alias analysis for omp private allocatable item #120243

DominikAdamski · 2024-12-17T15:21:53Z

Flang alias analysis crashes for omp private allocatable item. The issue is described here The corresponding MLIR code for omp private allocatable object contains omp.yield without any results. We should not implicitly assume that all omp.yield operations for omp.private contain results.

Flang alias analysis crashes for omp private allocatable item. The issue is described here: llvm#116954 The corrsponding MLIR code for omp private allocatable object contains omp.yield without any results. We should not implicitly assume that all omp.yield operations for omp.private contain results.

llvmbot · 2024-12-17T15:22:27Z

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

Author: Dominik Adamski (DominikAdamski)

Changes

Flang alias analysis crashes for omp private allocatable item. The issue is described here The corresponding MLIR code for omp private allocatable object contains omp.yield without any results. We should not implicitly assume that all omp.yield operations for omp.private contain results.

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

2 Files Affected:

(modified) flang/lib/Optimizer/Analysis/AliasAnalysis.cpp (+2)
(added) flang/test/Analysis/AliasAnalysis/alias-analysis-omp-private-allocatable.mlir (+50)

diff --git a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
index 0b0f83d024ce33..5c3cfb307fc93f 100644
--- a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
+++ b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
@@ -517,6 +517,8 @@ static Value getPrivateArg(omp::BlockArgOpenMPOpInterface &argIface,
           SymbolTable::lookupNearestSymbolFrom<omp::PrivateClauseOp>(
               op, cast<SymbolRefAttr>(opSym));
       privateOp.walk([&](omp::YieldOp yieldOp) {
+        if (!yieldOp.getResults().size())
+          return;
         // TODO Extend alias analysis if omp.yield points to
         // block argument value
         if (!yieldOp.getResults()[0].getDefiningOp())
diff --git a/flang/test/Analysis/AliasAnalysis/alias-analysis-omp-private-allocatable.mlir b/flang/test/Analysis/AliasAnalysis/alias-analysis-omp-private-allocatable.mlir
new file mode 100644
index 00000000000000..5116622364fad8
--- /dev/null
+++ b/flang/test/Analysis/AliasAnalysis/alias-analysis-omp-private-allocatable.mlir
@@ -0,0 +1,50 @@
+// Use --mlir-disable-threading so that the AA queries are serialized
+// as well as its diagnostic output.
+// RUN: fir-opt %s -pass-pipeline='builtin.module(func.func(test-fir-alias-analysis))' -split-input-file --mlir-disable-threading 2>&1 | FileCheck %s
+
+// Fortran code before simplification:
+// SUBROUTINE mysub(ns,ne)
+//      INTEGER  :: n
+//      REAL(KIND=8), DIMENSION(:), allocatable     :: ar1
+//      real(kind=8), dimension(20) :: ar2
+//      REAL(KIND=8), DIMENSION(20) :: d
+//
+//!$OMP parallel PRIVATE(ar1)
+//         d(1:1) = (/(DOT_PRODUCT(ar1(1:n), ar2(1:n)),n=1, 1)/)
+//!$OMP END parallel
+//   END SUBROUTINE
+
+// CHECK-LABEL: Testing : "testPrivateAllocatable"
+// CHECK: ar2#0 <-> ar1#0: NoAlias
+// CHECK: ar2#1 <-> ar1#0: NoAlias
+// CHECK: ar2#0 <-> ar1#1: NoAlias
+// CHECK: ar2#1 <-> ar1#1: NoAlias
+
+omp.private {type = private} @_QFmysubEar1_private_ref_box_heap_Uxf64 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>> alloc {
+^bb0(%arg0: !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>):
+  %0 = fir.alloca !fir.box<!fir.heap<!fir.array<?xf64>>> {bindc_name = "ar1", pinned, uniq_name = "_QFmysubEar1"}
+  %5:2 = hlfir.declare %0 {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = "_QFmysubEar1"} : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>) -> (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>)
+  omp.yield(%5#0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>)
+} dealloc {
+^bb0(%arg0: !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>):
+  omp.yield
+}
+func.func @testPrivateAllocatable(%arg0: !fir.ref<i32> {fir.bindc_name = "ns"}, %arg1: !fir.ref<i32> {fir.bindc_name = "ne"}) {
+  %0 = fir.dummy_scope : !fir.dscope
+  %1 = fir.alloca !fir.box<!fir.heap<!fir.array<?xf64>>> {bindc_name = "ar1", uniq_name = "_QFmysubEar1"}
+  %2 = fir.zero_bits !fir.heap<!fir.array<?xf64>>
+  %c0 = arith.constant 0 : index
+  %3 = fir.shape %c0 : (index) -> !fir.shape<1>
+  %4 = fir.embox %2(%3) : (!fir.heap<!fir.array<?xf64>>, !fir.shape<1>) -> !fir.box<!fir.heap<!fir.array<?xf64>>>
+  fir.store %4 to %1 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>
+  %5:2 = hlfir.declare %1 {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = "_QFmysubEar1"} : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>) -> (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>)
+  %c20 = arith.constant 20 : index
+  %6 = fir.alloca !fir.array<20xf64> {bindc_name = "ar2", uniq_name = "_QFmysubEar2"}
+  %7 = fir.shape %c20 : (index) -> !fir.shape<1>
+  %8:2 = hlfir.declare %6(%7) {uniq_name = "_QFmysubEar2", test.ptr="ar2" } : (!fir.ref<!fir.array<20xf64>>, !fir.shape<1>) -> (!fir.ref<!fir.array<20xf64>>, !fir.ref<!fir.array<20xf64>>)
+  omp.parallel private(@_QFmysubEar1_private_ref_box_heap_Uxf64 %5#0 -> %arg2 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>) {
+    %20:2 = hlfir.declare %arg2 {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = "_QFmysubEar1", test.ptr = "ar1"} : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>) -> (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>)
+    omp.terminator
+  }
+  return
+}

kiranchandramohan

LG. Please wait for @tblah

kiranchandramohan · 2024-12-17T16:49:35Z

flang/test/Analysis/AliasAnalysis/alias-analysis-omp-private-allocatable.mlir

+func.func @testPrivateAllocatable(%arg0: !fir.ref<i32> {fir.bindc_name = "ns"}, %arg1: !fir.ref<i32> {fir.bindc_name = "ne"}) {
+  %0 = fir.dummy_scope : !fir.dscope
+  %1 = fir.alloca !fir.box<!fir.heap<!fir.array<?xf64>>> {bindc_name = "ar1", uniq_name = "_QFmysubEar1"}
+  %2 = fir.zero_bits !fir.heap<!fir.array<?xf64>>
+  %c0 = arith.constant 0 : index
+  %3 = fir.shape %c0 : (index) -> !fir.shape<1>
+  %4 = fir.embox %2(%3) : (!fir.heap<!fir.array<?xf64>>, !fir.shape<1>) -> !fir.box<!fir.heap<!fir.array<?xf64>>>
+  fir.store %4 to %1 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>
+  %5:2 = hlfir.declare %1 {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = "_QFmysubEar1"} : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>) -> (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>)
+  %c20 = arith.constant 20 : index
+  %6 = fir.alloca !fir.array<20xf64> {bindc_name = "ar2", uniq_name = "_QFmysubEar2"}
+  %7 = fir.shape %c20 : (index) -> !fir.shape<1>
+  %8:2 = hlfir.declare %6(%7) {uniq_name = "_QFmysubEar2", test.ptr="ar2" } : (!fir.ref<!fir.array<20xf64>>, !fir.shape<1>) -> (!fir.ref<!fir.array<20xf64>>, !fir.ref<!fir.array<20xf64>>)
+  omp.parallel private(@_QFmysubEar1_private_ref_box_heap_Uxf64 %5#0 -> %arg2 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>) {
+    %20:2 = hlfir.declare %arg2 {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = "_QFmysubEar1", test.ptr = "ar1"} : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>) -> (!fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xf64>>>>)
+    omp.terminator
+  }
+  return
+}


Do we need this func.func code for the test to work? Or is the omp.private op only required?

Hi,
We need code inside func.fun for the test to work.

Detailed explanation:
fun.func MLIR code contains two operations:
%20:2 = hlfir.declare %arg2 {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = "_QFmysubEar1", test.ptr = "ar1"}
and:
%8:2 = hlfir.declare %6(%7) {uniq_name = "_QFmysubEar2", test.ptr="ar2" }
These two operations contain manually added test.ptr attributes. test.ptr attributes are required for testing alias analysis. If we don't attach them, we don't see the console output of alias analysis (i.e.: ar2#0 <-> ar1#0: NoAlias )

tblah · 2024-12-17T18:16:22Z

Thanks for looking into this bug. I think the real problem here is that all of the regions inside of the private op are being inspected. If I understand correctly, the intention is for getPrivateArg to find the allocation for this private argument. But as it is written now it is checking all of the regions looking for a Yield operation and so it will also look at dealloc and copy regions (as well as the alloc region that I think was the intention). I think the iteration order over regions inside of an mlir operation is not defined.

I think the correct solution is to only look for yield operations inside of the alloc region. This is guaranteed to yield a value.

Please also change the code so that it detects multiple omp.yield operations yielding different values. If there are multiple possible values yielded then we cannot know which one will happen at runtime (assuming no dead code).

Or maybe even some simpler logic is possible. I don't think the descriptor for a private value can alias with anything. I think the value of the private value can't alias with anything else unless it is POINTER or TARGET.

DominikAdamski · 2024-12-19T09:54:27Z

Or maybe even some simpler logic is possible. I don't think the descriptor for a private value can alias with anything. I think the value of the private value can't alias with anything else unless it is POINTER or TARGET.

@tblah I simplified logic according to your suggestion. Test for POINTER is already present: https://github.com/llvm/llvm-project/blob/main/flang/test/Analysis/AliasAnalysis/alias-analysis-omp-teams-distribute-private-ptr.mlir , that's why I attach only test for allocatable.

shivaramaarao · 2024-12-24T16:11:02Z

veified that this PR fixes the original issue. thanks.

tblah

Thanks for the update!

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Dec 17, 2024

DominikAdamski requested a review from tblah December 17, 2024 15:22

DominikAdamski requested review from kiranchandramohan and kiranktp December 17, 2024 15:22

DominikAdamski mentioned this pull request Dec 17, 2024

[flang] compiler crashes in OptimizedBufferizationPass when dot_product is used inside parallel region #116954

Closed

kiranchandramohan approved these changes Dec 17, 2024

View reviewed changes

Applied remarks

07d4bb8

tblah approved these changes Jan 2, 2025

View reviewed changes

DominikAdamski merged commit 5236e3d into llvm:main Jan 2, 2025
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Flang][Alias analysis] Fix alias analysis for omp private allocatable item #120243

[Flang][Alias analysis] Fix alias analysis for omp private allocatable item #120243

Uh oh!

DominikAdamski commented Dec 17, 2024

Uh oh!

llvmbot commented Dec 17, 2024

Uh oh!

kiranchandramohan left a comment

Uh oh!

kiranchandramohan Dec 17, 2024

Uh oh!

DominikAdamski Dec 19, 2024

Uh oh!

tblah commented Dec 17, 2024

Uh oh!

DominikAdamski commented Dec 19, 2024

Uh oh!

shivaramaarao commented Dec 24, 2024

Uh oh!

tblah left a comment

Uh oh!

Uh oh!

Uh oh!

[Flang][Alias analysis] Fix alias analysis for omp private allocatable item #120243

[Flang][Alias analysis] Fix alias analysis for omp private allocatable item #120243

Uh oh!

Conversation

DominikAdamski commented Dec 17, 2024

Uh oh!

llvmbot commented Dec 17, 2024

Uh oh!

kiranchandramohan left a comment

Choose a reason for hiding this comment

Uh oh!

kiranchandramohan Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

DominikAdamski Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

tblah commented Dec 17, 2024

Uh oh!

DominikAdamski commented Dec 19, 2024

Uh oh!

shivaramaarao commented Dec 24, 2024

Uh oh!

tblah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!