[flang][Alias Analysis] not all block arguments are dummy arguments

[tblah]

Arguments to openmp regions should not be tagged as dummy arguments. This is particularly unsafe because these openmp blocks will eventually be inlined into the calling function, where they will trivially alias with other values inside of the calling function.

This is probably a theoretical issue because the calls to openmp runtime function calls would act as barriers, preventing optimizations that are too aggressive. But a lot more thought would need to go into a bet like that.

This came out of discussion on
#92036

[llvmbot]

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

Author: Tom Eccles (tblah)

Changes

Arguments to openmp regions should not be tagged as dummy arguments. This is particularly unsafe because these openmp blocks will eventually be inlined into the calling function, where they will trivially alias with other values inside of the calling function.

This is probably a theoretical issue because the calls to openmp runtime function calls would act as barriers, preventing optimizations that are too aggressive. But a lot more thought would need to go into a bet like that.

This came out of discussion on
#92036

---

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

3 Files Affected:

• (modified) flang/lib/Optimizer/Analysis/AliasAnalysis.cpp (+5-1)
• (modified) flang/lib/Optimizer/Transforms/AddAliasTags.cpp (+1)
• (modified) flang/test/Transforms/tbaa.fir (+37)

diff --git a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
index f723e8f66e3e4..ed1101dc5e8d8 100644
--- a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
+++ b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
@@ -33,7 +33,11 @@ static bool isDummyArgument(mlir::Value v) {
   if (!blockArg)
     return false;
 
-  return blockArg.getOwner()->isEntryBlock();
+  mlir::Block *owner = blockArg.getOwner();
+  if (!owner->isEntryBlock() ||
+      !mlir::isa<mlir::FunctionOpInterface>(owner->getParentOp()))
+    return false;
+  return true;
 }
 
 /// Temporary function to skip through all the no op operations
diff --git a/flang/lib/Optimizer/Transforms/AddAliasTags.cpp b/flang/lib/Optimizer/Transforms/AddAliasTags.cpp
index 684aa4462915e..3642a812096db 100644
--- a/flang/lib/Optimizer/Transforms/AddAliasTags.cpp
+++ b/flang/lib/Optimizer/Transforms/AddAliasTags.cpp
@@ -105,6 +105,7 @@ static std::string getFuncArgName(mlir::Value arg) {
          "arg is a function argument");
   mlir::FunctionOpInterface func = mlir::dyn_cast<mlir::FunctionOpInterface>(
       blockArg.getOwner()->getParentOp());
+  assert(func && "This is not a function argument");
   mlir::StringAttr attr = func.getArgAttrOfType<mlir::StringAttr>(
       blockArg.getArgNumber(), "fir.bindc_name");
   if (!attr)
diff --git a/flang/test/Transforms/tbaa.fir b/flang/test/Transforms/tbaa.fir
index 7825ae60c71e6..f94bbe4bf9485 100644
--- a/flang/test/Transforms/tbaa.fir
+++ b/flang/test/Transforms/tbaa.fir
@@ -173,3 +173,40 @@
 // CHECK:           fir.store %[[VAL_8]] to %[[VAL_12]] : !fir.ref<i32>
 // CHECK:           return
 // CHECK:         }
+
+// -----
+
+// Make sure we don't mistake other block arguments as dummy arguments:
+
+omp.declare_reduction @add_reduction_i32 : i32 init {
+^bb0(%arg0: i32):
+  %c0_i32 = arith.constant 0 : i32
+  omp.yield(%c0_i32 : i32)
+} combiner {
+^bb0(%arg0: i32, %arg1: i32):
+  %0 = arith.addi %arg0, %arg1 : i32
+  omp.yield(%0 : i32)
+}
+
+func.func @_QQmain() attributes {fir.bindc_name = "reduce"} {
+  %c10_i32 = arith.constant 10 : i32
+  %c6_i32 = arith.constant 6 : i32
+  %c-1_i32 = arith.constant -1 : i32
+  %0 = fir.address_of(@_QFEi) : !fir.ref<i32>
+  %1 = fir.declare %0 {uniq_name = "_QFEi"} : (!fir.ref<i32>) -> !fir.ref<i32>
+  omp.parallel reduction(@add_reduction_i32 %1 -> %arg0 : !fir.ref<i32>) {
+// CHECK: omp.parallel reduction({{.*}}) {
+    %8 = fir.declare %arg0 {uniq_name = "_QFEi"} : (!fir.ref<i32>) -> !fir.ref<i32>
+// CHECK-NEXT: %[[DECL:.*]] = fir.declare
+    fir.store %c-1_i32 to %8 : !fir.ref<i32>
+// CHECK-NOT: fir.store %{{.*}} to %[[DECL]] {tbaa = %{{.*}}} : !fir.ref<i32>
+// CHECK:     fir.store %{{.*}} to %[[DECL]] : !fir.ref<i32>
+    omp.terminator
+  }
+  return
+}
+
+fir.global internal @_QFEi : i32 {
+  %c0_i32 = arith.constant 0 : i32
+  fir.has_value %c0_i32 : i32
+}

[Renaud-K]

This is the same change Joel was trying to make in his PR, which I also applied in mine.
It will be trivial to resolve.
LGTM.

[agozillon]

LGTM, also tested it downstream and it seems to resolve the issue we encountered! Thank you very much for the PR.