[flang-commits] [flang] [flang][Alias Analysis] not all block arguments are dummy arguments (PR #92156)

[Tom Eccles via flang-commits]

https://github.com/tblah created https://github.com/llvm/llvm-project/pull/92156

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
https://github.com/llvm/llvm-project/pull/92036

>From bb5d4156c9bff986bf75504f6fb41667c45b09b9 Mon Sep 17 00:00:00 2001
From: Tom Eccles <tom.eccles at arm.com>
Date: Tue, 14 May 2024 18:06:07 +0000
Subject: [PATCH] [flang][Alias Analysis] not all block arguments are dummy
 arguments

Arguments to openmp regions should not be treated 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
https://github.com/llvm/llvm-project/pull/92036
---
 .../lib/Optimizer/Analysis/AliasAnalysis.cpp  |  6 ++-
 .../lib/Optimizer/Transforms/AddAliasTags.cpp |  1 +
 flang/test/Transforms/tbaa.fir                | 37 +++++++++++++++++++
 3 files changed, 43 insertions(+), 1 deletion(-)

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
+}