[flang] handle fir.call in AliasAnalysis::getModRef #117164
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeanPerier
requested review from
razvanlupusoru,
clementval,
tblah,
klausler,
Renaud-K and
vzakhari
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: None (jeanPerier) Changesfir.call side effects are hard to describe in a useful way using While doing a data flow analysis is likely unavoidable at some point, it will not address cases where the procedure body is not available in the current compilation unit, and will be rather expansive to do. Luckily, Fortran language specifications allow the compiler to deduce that a procedure call cannot access a variable in many cases (this mainly stems from the 15.5.2.14 restrictions about dummy argument, and the inability to capture variables that do not have the TARGET attribute). MLIR provides the perfect interface to leverages that: This patch extends
It then:
Currently, it is always replying "ModRef" (may be referenced or modified) or "NoModRef" (may nor be referenced neither modified). This could be refined in the future to reply "Ref" for the cases where the only access is made via "Intent(IN)" argument. It also inherits a lot of "false positive cases" coming from alias analysis current limitations (e.g., any copy-in/out on an arguments will make it return "ModRef" because alias analysis currently does not handle hlfir.copy_in in the SSA chain). These will be improved with time. @klausler, I am adding you as a reviewer for the Fortran test (not the implementation) because it is very important that I am getting the language specifications correct here. Any This will allow implementing "array = array_function()" optimization in a future patch. Patch is 33.65 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/117164.diff 14 Files Affected:
diff --git a/flang/include/flang/Optimizer/Analysis/AliasAnalysis.h b/flang/include/flang/Optimizer/Analysis/AliasAnalysis.h
index d9953f580f401d..e410831c0fc3eb 100644
--- a/flang/include/flang/Optimizer/Analysis/AliasAnalysis.h
+++ b/flang/include/flang/Optimizer/Analysis/AliasAnalysis.h
@@ -129,7 +129,7 @@ struct AliasAnalysis {
/// inlining happens an inlined fir.declare of the callee's
/// dummy argument identifies the scope where the source
/// may be treated as a dummy argument.
- mlir::Value instantiationPoint;
+ mlir::Operation *instantiationPoint;
/// Whether the source was reached following data or box reference
bool isData{false};
@@ -146,6 +146,8 @@ struct AliasAnalysis {
/// Have we lost precision following the source such that
/// even an exact match cannot be MustAlias?
bool approximateSource;
+ /// Source object is used in an internal procedure via host association.
+ bool isCapturedInInternalProcedure{false};
/// Print information about the memory source to `os`.
void print(llvm::raw_ostream &os) const;
@@ -157,6 +159,9 @@ struct AliasAnalysis {
bool isData() const;
bool isBoxData() const;
+ /// Is this source a variable from the Fortran source?
+ bool isFortranUserVariable() const;
+
/// @name Dummy Argument Aliasing
///
/// Check conditions related to dummy argument aliasing.
@@ -194,11 +199,11 @@ struct AliasAnalysis {
mlir::ModRefResult getModRef(mlir::Operation *op, mlir::Value location);
/// Return the memory source of a value.
- /// If getInstantiationPoint is true, the search for the source
+ /// If getLastInstantiationPoint is true, the search for the source
/// will stop at [hl]fir.declare if it represents a dummy
/// argument declaration (i.e. it has the dummy_scope operand).
fir::AliasAnalysis::Source getSource(mlir::Value,
- bool getInstantiationPoint = false);
+ bool getLastInstantiationPoint = false);
private:
/// Return true, if `ty` is a reference type to an object of derived type
diff --git a/flang/include/flang/Optimizer/Dialect/FortranVariableInterface.td b/flang/include/flang/Optimizer/Dialect/FortranVariableInterface.td
index 926e00ca043407..0fe2e60a1a95cc 100644
--- a/flang/include/flang/Optimizer/Dialect/FortranVariableInterface.td
+++ b/flang/include/flang/Optimizer/Dialect/FortranVariableInterface.td
@@ -184,6 +184,13 @@ def fir_FortranVariableOpInterface : OpInterface<"FortranVariableOpInterface"> {
fir::FortranVariableFlagsEnum::target);
}
+ /// Is this variable captured in an internal procedure via Fortran host association?
+ bool isCapturedInInternalProcedure() {
+ auto attrs = getFortranAttrs();
+ return attrs && bitEnumContainsAny(*attrs,
+ fir::FortranVariableFlagsEnum::internal_assoc);
+ }
+
/// Is this variable a Fortran intent(in)?
bool isIntentIn() {
auto attrs = getFortranAttrs();
diff --git a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
index 0c2e37c4446aa0..cfe953dad24674 100644
--- a/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
+++ b/flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
@@ -12,6 +12,7 @@
#include "flang/Optimizer/Dialect/FIRType.h"
#include "flang/Optimizer/Dialect/FortranVariableInterface.h"
#include "flang/Optimizer/HLFIR/HLFIROps.h"
+#include "flang/Optimizer/Support/InternalNames.h"
#include "mlir/Analysis/AliasAnalysis.h"
#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
#include "mlir/Dialect/OpenMP/OpenMPInterfaces.h"
@@ -96,6 +97,17 @@ bool AliasAnalysis::Source::isBoxData() const {
origin.isData;
}
+bool AliasAnalysis::Source::isFortranUserVariable() const {
+ if (!origin.instantiationPoint)
+ return false;
+ return llvm::TypeSwitch<mlir::Operation *, bool>(origin.instantiationPoint)
+ .template Case<fir::DeclareOp, hlfir::DeclareOp>([&](auto declOp) {
+ return fir::NameUniquer::deconstruct(declOp.getUniqName()).first ==
+ fir::NameUniquer::NameKind::VARIABLE;
+ })
+ .Default([&](auto op) { return false; });
+}
+
bool AliasAnalysis::Source::mayBeDummyArgOrHostAssoc() const {
return kind != SourceKind::Allocate && kind != SourceKind::Global;
}
@@ -329,14 +341,92 @@ AliasResult AliasAnalysis::alias(Source lhsSrc, Source rhsSrc, mlir::Value lhs,
// AliasAnalysis: getModRef
//===----------------------------------------------------------------------===//
+static bool isSavedLocal(const fir::AliasAnalysis::Source &src) {
+ if (auto symRef = llvm::dyn_cast<mlir::SymbolRefAttr>(src.origin.u)) {
+ auto [nameKind, deconstruct] =
+ fir::NameUniquer::deconstruct(symRef.getLeafReference().getValue());
+ return nameKind == fir::NameUniquer::NameKind::VARIABLE &&
+ !deconstruct.procs.empty();
+ }
+ return false;
+}
+
+static bool isCallToFortranUserProcedure(fir::CallOp call) {
+ // TODO: indirect calls are excluded by these checks. Maybe some attribute is
+ // needed to flag user calls in this case.
+ if (fir::hasBindcAttr(call))
+ return true;
+ if (std::optional<mlir::SymbolRefAttr> callee = call.getCallee())
+ return fir::NameUniquer::deconstruct(callee->getLeafReference().getValue())
+ .first == fir::NameUniquer::NameKind::PROCEDURE;
+ return false;
+}
+
+static ModRefResult getCallModRef(fir::CallOp call, mlir::Value var) {
+ // TODO: limit to Fortran functions??
+ // 1. Detect variables that can be accessed indirectly.
+ fir::AliasAnalysis aliasAnalysis;
+ fir::AliasAnalysis::Source varSrc = aliasAnalysis.getSource(var);
+ // If the variable is not a user variable, we cannot safely assume that
+ // Fortran semantics apply (e.g., a bare alloca/allocmem result may very well
+ // be placed in an allocatable/pointer descriptor and escape).
+
+ // All the logic bellows are based on Fortran semantics and only holds if this
+ // is a call to a procedure form the Fortran source and this is a variable
+ // from the Fortran source. Compiler generated temporaries or functions may
+ // not adhere to this semantic.
+ // TODO: add some opt-in or op-out mechanism for compiler generated temps.
+ // An example of something currently problematic is the allocmem generated for
+ // ALLOCATE of allocatable target. It currently does not have the target
+ // attribute, which would lead this analysis to believe it cannot escape.
+ if (!varSrc.isFortranUserVariable() || !isCallToFortranUserProcedure(call))
+ return ModRefResult::getModAndRef();
+ // Pointer and target may have been captured.
+ if (varSrc.isTargetOrPointer())
+ return ModRefResult::getModAndRef();
+ // Host associated variables may be addressed indirectly via an internal
+ // function call, whether the call is in the parent or an internal procedure.
+ // Note that the host associated/internal procedure may be referenced
+ // indirectly inside calls to non internal procedure. This is because internal
+ // procedures may be captured or passed. As this is tricky to analyze, always
+ // consider such variables may be accessed in any calls.
+ if (varSrc.kind == fir::AliasAnalysis::SourceKind::HostAssoc ||
+ varSrc.isCapturedInInternalProcedure)
+ return ModRefResult::getModAndRef();
+ // At that stage, it has been ruled out that local (including the saved ones)
+ // and dummy cannot be indirectly accessed in the call.
+ if (varSrc.kind != fir::AliasAnalysis::SourceKind::Allocate &&
+ !varSrc.isDummyArgument()) {
+ if (varSrc.kind != fir::AliasAnalysis::SourceKind::Global ||
+ !isSavedLocal(varSrc))
+ return ModRefResult::getModAndRef();
+ }
+ // 2. Check if the variable is passed via the arguments.
+ for (auto arg : call.getArgs()) {
+ if (fir::conformsWithPassByRef(arg.getType()) &&
+ !aliasAnalysis.alias(arg, var).isNo()) {
+ // TODO: intent(in) would allow returning Ref here. This can be obtained
+ // in the func.func attributes for direct calls, but the module lookup is
+ // linear with the number of MLIR symbols, which would introduce a pseudo
+ // quadratic behavior num_calls * num_func.
+ return ModRefResult::getModAndRef();
+ }
+ }
+ // The call cannot access the variable.
+ return ModRefResult::getNoModRef();
+}
+
/// This is mostly inspired by MLIR::LocalAliasAnalysis with 2 notable
/// differences 1) Regions are not handled here but will be handled by a data
/// flow analysis to come 2) Allocate and Free effects are considered
/// modifying
ModRefResult AliasAnalysis::getModRef(Operation *op, Value location) {
MemoryEffectOpInterface interface = dyn_cast<MemoryEffectOpInterface>(op);
- if (!interface)
+ if (!interface) {
+ if (auto call = llvm::dyn_cast<fir::CallOp>(op))
+ return getCallModRef(call, location);
return ModRefResult::getModAndRef();
+ }
// Build a ModRefResult by merging the behavior of the effects of this
// operation.
@@ -408,19 +498,20 @@ static Value getPrivateArg(omp::BlockArgOpenMPOpInterface &argIface,
}
AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
- bool getInstantiationPoint) {
+ bool getLastInstantiationPoint) {
auto *defOp = v.getDefiningOp();
SourceKind type{SourceKind::Unknown};
mlir::Type ty;
bool breakFromLoop{false};
bool approximateSource{false};
+ bool isCapturedInInternalProcedure{false};
bool followBoxData{mlir::isa<fir::BaseBoxType>(v.getType())};
bool isBoxRef{fir::isa_ref_type(v.getType()) &&
mlir::isa<fir::BaseBoxType>(fir::unwrapRefType(v.getType()))};
bool followingData = !isBoxRef;
mlir::SymbolRefAttr global;
Source::Attributes attributes;
- mlir::Value instantiationPoint;
+ mlir::Operation *instantiationPoint{nullptr};
while (defOp && !breakFromLoop) {
ty = defOp->getResultTypes()[0];
llvm::TypeSwitch<Operation *>(defOp)
@@ -548,6 +639,8 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
// is the only carrier of the variable attributes,
// so we have to collect them here.
attributes |= getAttrsFromVariable(varIf);
+ isCapturedInInternalProcedure |=
+ varIf.isCapturedInInternalProcedure();
if (varIf.isHostAssoc()) {
// Do not track past such DeclareOp, because it does not
// currently provide any useful information. The host associated
@@ -561,10 +654,10 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
breakFromLoop = true;
return;
}
- if (getInstantiationPoint) {
+ if (getLastInstantiationPoint) {
// Fetch only the innermost instantiation point.
if (!instantiationPoint)
- instantiationPoint = op->getResult(0);
+ instantiationPoint = op;
if (op.getDummyScope()) {
// Do not track past DeclareOp that has the dummy_scope
@@ -575,6 +668,8 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
breakFromLoop = true;
return;
}
+ } else {
+ instantiationPoint = op;
}
// TODO: Look for the fortran attributes present on the operation
// Track further through the operand
@@ -620,13 +715,15 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v,
type,
ty,
attributes,
- approximateSource};
+ approximateSource,
+ isCapturedInInternalProcedure};
}
return {{v, instantiationPoint, followingData},
type,
ty,
attributes,
- approximateSource};
+ approximateSource,
+ isCapturedInInternalProcedure};
}
} // namespace fir
diff --git a/flang/lib/Optimizer/Analysis/CMakeLists.txt b/flang/lib/Optimizer/Analysis/CMakeLists.txt
index c000a9da99f871..1358219fd98d52 100644
--- a/flang/lib/Optimizer/Analysis/CMakeLists.txt
+++ b/flang/lib/Optimizer/Analysis/CMakeLists.txt
@@ -4,6 +4,7 @@ add_flang_library(FIRAnalysis
DEPENDS
FIRDialect
+ FIRSupport
HLFIRDialect
MLIRIR
MLIROpenMPDialect
diff --git a/flang/lib/Optimizer/Transforms/AddAliasTags.cpp b/flang/lib/Optimizer/Transforms/AddAliasTags.cpp
index 8feba072cfea67..f1e70875de0ba7 100644
--- a/flang/lib/Optimizer/Transforms/AddAliasTags.cpp
+++ b/flang/lib/Optimizer/Transforms/AddAliasTags.cpp
@@ -209,12 +209,11 @@ void AddAliasTagsPass::runOnAliasInterface(fir::FirAliasTagOpInterface op,
state.processFunctionScopes(func);
fir::DummyScopeOp scopeOp;
- if (auto declVal = source.origin.instantiationPoint) {
+ if (auto declOp = source.origin.instantiationPoint) {
// If the source is a dummy argument within some fir.dummy_scope,
// then find the corresponding innermost scope to be used for finding
// the right TBAA tree.
- auto declareOp =
- mlir::dyn_cast_or_null<fir::DeclareOp>(declVal.getDefiningOp());
+ auto declareOp = mlir::dyn_cast<fir::DeclareOp>(declOp);
assert(declareOp && "Instantiation point must be fir.declare");
if (auto dummyScope = declareOp.getDummyScope())
scopeOp = mlir::cast<fir::DummyScopeOp>(dummyScope.getDefiningOp());
diff --git a/flang/test/Analysis/AliasAnalysis/gen_mod_ref_test.py b/flang/test/Analysis/AliasAnalysis/gen_mod_ref_test.py
new file mode 100755
index 00000000000000..92a38f727fd80a
--- /dev/null
+++ b/flang/test/Analysis/AliasAnalysis/gen_mod_ref_test.py
@@ -0,0 +1,18 @@
+#!/usr/bin/env python3
+
+"""
+ Add attributes hook in an HLFIR code to test fir.call ModRef effects
+ with the test-fir-alias-analysis-modref pass.
+
+ This will insert mod ref test hook:
+ - to any fir.call to a function which name starts with "test_effect_"
+ - to any hlfir.declare for variable which name starts with "test_var_"
+"""
+
+import sys
+import re
+
+for line in sys.stdin:
+ line = re.sub(r'(fir.call @_\w*P)(test_effect_\w*)(\(.*) : ', r'\1\2\3 {test.ptr ="\2"} : ', line)
+ line = re.sub(r'(hlfir.declare .*uniq_name =.*E)(test_var_\w*)"', r'\1\2", test.ptr ="\2"', line)
+ sys.stdout.write(line)
diff --git a/flang/test/Analysis/AliasAnalysis/modref-call-after-inlining.fir b/flang/test/Analysis/AliasAnalysis/modref-call-after-inlining.fir
new file mode 100644
index 00000000000000..c9dd03c95d7e87
--- /dev/null
+++ b/flang/test/Analysis/AliasAnalysis/modref-call-after-inlining.fir
@@ -0,0 +1,45 @@
+// RUN: fir-opt -pass-pipeline='builtin.module(func.func(test-fir-alias-analysis-modref))' \
+// RUN: --mlir-disable-threading %s -o /dev/null 2>&1 | FileCheck %s
+
+// Test fir.call modref with internal procedures after the host function has been inlined in
+// some other function. This checks that the last hlfir.declare "internal_assoc" flags that
+// marks a variable that was captured is still considered even though there is no such flags
+// on the declare at the top of the chain.
+//
+// In other words, in the following Fortran example, "x" should be considered
+// modified by "call internal_proc" after "call inline_me" was inlined into
+// "test".
+//
+// subroutine test()
+// real :: x(10)
+// call inline_me(x)
+// end subroutine
+//
+// subroutine inline_me(x)
+// real :: x(10)
+// call internal_proc()
+// contains
+// subroutine internal_proc()
+// call some_external(x)
+// end subroutine
+// end subroutine
+
+func.func @_QPtest() {
+ %c0_i32 = arith.constant 0 : i32
+ %c10 = arith.constant 10 : index
+ %0 = fir.alloca !fir.array<10xf32> {bindc_name = "x", uniq_name = "_QFtestEx"}
+ %1 = fir.shape %c10 : (index) -> !fir.shape<1>
+ %2:2 = hlfir.declare %0(%1) {uniq_name = "_QFtestEx"} : (!fir.ref<!fir.array<10xf32>>, !fir.shape<1>) -> (!fir.ref<!fir.array<10xf32>>, !fir.ref<!fir.array<10xf32>>)
+ %3 = fir.dummy_scope : !fir.dscope
+ %4:2 = hlfir.declare %2#1(%1) dummy_scope %3 {test.ptr = "x", fortran_attrs = #fir.var_attrs<internal_assoc>, uniq_name = "_QFinline_meEx"} : (!fir.ref<!fir.array<10xf32>>, !fir.shape<1>, !fir.dscope) -> (!fir.ref<!fir.array<10xf32>>, !fir.ref<!fir.array<10xf32>>)
+ %5 = fir.alloca tuple<!fir.box<!fir.array<10xf32>>>
+ %6 = fir.coordinate_of %5, %c0_i32 : (!fir.ref<tuple<!fir.box<!fir.array<10xf32>>>>, i32) -> !fir.ref<!fir.box<!fir.array<10xf32>>>
+ %7 = fir.embox %4#1(%1) : (!fir.ref<!fir.array<10xf32>>, !fir.shape<1>) -> !fir.box<!fir.array<10xf32>>
+ fir.store %7 to %6 : !fir.ref<!fir.box<!fir.array<10xf32>>>
+ fir.call @_QFinline_mePinternal_proc(%5) {test.ptr="internal_proc"} : (!fir.ref<tuple<!fir.box<!fir.array<10xf32>>>>) -> ()
+ return
+}
+func.func private @_QFinline_mePinternal_proc(!fir.ref<tuple<!fir.box<!fir.array<10xf32>>>> {fir.host_assoc}) attributes {fir.host_symbol = @_QPinline_me}
+
+// CHECK-LABEL: Testing : "_QPtest"
+// CHECK: internal_proc -> x#0: ModRef
diff --git a/flang/test/Analysis/AliasAnalysis/modref-call-args.f90 b/flang/test/Analysis/AliasAnalysis/modref-call-args.f90
new file mode 100644
index 00000000000000..5fc2b8143377b0
--- /dev/null
+++ b/flang/test/Analysis/AliasAnalysis/modref-call-args.f90
@@ -0,0 +1,62 @@
+! RUN: bbc -emit-hlfir %s -o - | %python %S/gen_mod_ref_test.py | \
+! RUN: fir-opt -pass-pipeline='builtin.module(func.func(test-fir-alias-analysis-modref))' \
+! RUN: --mlir-disable-threading -o /dev/null 2>&1 | FileCheck %s
+
+! Test fir.call modref when arguments are passed to the call. This focus
+! on the possibility of "direct" effects (taken via the arguments, and not
+! via some indirect access via global states).
+
+subroutine test_simple()
+ implicit none
+ real :: test_var_x, test_var_y
+ call test_effect_external(test_var_x)
+end subroutine
+! CHECK-LABEL: Testing : "_QPtest_simple"
+! CHECK: test_effect_external -> test_var_x#0: ModRef
+! CHECK: test_effect_external -> test_var_y#0: NoModRef
+
+subroutine test_equivalence()
+ implicit none
+ real :: test_var_x, test_var_y
+ equivalence(test_var_x, test_var_y)
+ call test_effect_external(test_var_x)
+end subroutine
+! CHECK-LABEL: Testing : "_QPtest_equivalence"
+! CHECK: test_effect_external -> test_var_x#0: ModRef
+! CHECK: test_effect_external -> test_var_y#0: ModRef
+
+subroutine test_pointer()
+ implicit none
+ real, target :: test_var_x, test_var_y
+ real, pointer :: p
+ p => test_var_x
+ call test_effect_external(p)
+end subroutine
+! CHECK-LABEL: Testing : "_QPtest_pointer"
+! CHECK: test_effect_external -> test_var_x#0: ModRef
+! TODO: test_var_y should be NoModRef, the alias analysis is currently very
+! conservative whenever pointer/allocatable descriptors are involved (mostly
+! because it needs to make sure it is dealing descriptors for POINTER/ALLOCATABLE
+! from the Fortran source and that it can apply language rules).
+! CHECK: test_effect_external -> test_var_y#0: ModRef
+
+subroutine test_array_1(test_var_x)
+ implicit none
+ real :: test_var_x(:), test_var_y
+ call test_effect_external(test_var_x(10))
+end subroutine
+! CHECK-LABEL: Testing : "_QPtest_array_1"
+! CHECK: test_effect_external -> test_var_x#0: ModRef
+! CHECK: test_effect_external -> test_var_y#0: NoModRef
+
+subroutine test_array_copy_in(test_var_x)
+ implicit none
+ real :: test_var_x(:), test_var_y
+ call test_effect_external_2(test_var_x)
+end subroutine
+! CHECK-LABEL: Testing : "_QPtest_array_copy_in"
+! CHECK: test_effect_external_2 -> test_var_x#0: ModRef
+! TODO: copy-in/out is currently badly understood by alias analysis, this
+! causes the modref analysis to think the argument may alias with anyting.
+! test_var_y should obviously be considered NoMoRef in the call.
+! CHECK: test_effect_external_2 -> test_var_y#0: ModRef
diff --git a/flang/test/Analysis/AliasAnalysis/modref-call-dummies.f90 b/flang/test/Analysis/AliasAnalysis/modref-call-dummies.f90
new file mode 100644
index 00000000000000..a4c57cff70927f
--- /dev/null
+++ b/flang/test/Analysis/AliasAnalysis/modref-call-dummies....
[truncated]
|
|
✅ With the latest revision this PR passed the Python code formatter. |
tblah
approved these changes
Nov 21, 2024
Comment on lines
+408
to
+411
| // TODO: intent(in) would allow returning Ref here. This can be obtained | ||
| // in the func.func attributes for direct calls, but the module lookup is | ||
| // linear with the number of MLIR symbols, which would introduce a pseudo | ||
| // quadratic behavior num_calls * num_func. |
There was a problem hiding this comment.
I believe lookups in an mlir::SymbolTable are constant time. Constructing a SymbolTable is linear, but perhaps one could be re-used from a calling context. Or fir::AliasAnalysis could have a LazySymbolTable (AbstractResult.cpp).
It is fine by me to leave this as a TODO in this PR and only attempt this if the optimization turns out to be useful on some real code.
There was a problem hiding this comment.
fir::AliasAnalysis could have a LazySymbolTable.
Makes some sense to me. The only limitation I see here is that any changes to the ModuleOp symbols (name change/new functions) would not propagate to it, so fir::AliasAnalysis users would have to ensure they do not modify symbols during the lifetime of the AliasAnalysis object (or to somehow update the symbol table too).
I am planning to chase the moduleOp lookups at some point and try to think of a way to keep and maintain symbol tables in the pipeline.
razvanlupusoru
approved these changes
Nov 21, 2024
There was a problem hiding this comment.
Looks amazing! I agree with the various limitations and as far as I can tell - the non-implemented TODOs are not a correctness problem - just a limitation.
Do you have plans to add support for Fortran runtime calls also? I think a similar approach as your check for escaping args would work conservatively for them as well.
klausler
approved these changes
Nov 21, 2024
flang/test/Analysis/AliasAnalysis/modref-call-internal-proc.f90
Outdated
Show resolved
Hide resolved
|
Thanks for the reviews!
Yes, I am planning to do that in a later patch. I think the only difference is that when checking the argument aliasing, I am relying on the fact that the address cannot be "hidden" inside a descriptor reference because POINTER and TARGET variables were excluded before. This is not true when calling the runtime (For instance, Assign destination argument is always a fir.ref<fir.box> and we do place the address of non TARGET variable inside that descriptor). So the argument checking will have to be stricter with descriptor addresses. Hence I want to think it properly and make it in its own patch. |
jeanPerier
added a commit
that referenced
this pull request
Dec 3, 2024
This patch encapsulate array function call lowering into hlfir.eval_in_mem and allows directly evaluating the call into the LHS when possible. The conditions are: LHS is contiguous, not accessed inside the function, it is not a whole allocatable, and the function results needs not to be finalized. All these conditions are tested in the previous hlfir.eval_in_mem optimization (#118069) that is leveraging the extension of getModRef to handle function calls(#117164). This yields a 25% speed-up on polyhedron channel2 benchmark (from 1min to 45s measured on an X86-64 Zen 2).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fir.call side effects are hard to describe in a useful way using
MemoryEffectOpInterfacebecause it is impossible to list which memory location a user procedure read/write without doing a data flow analysis of its body (even PURE procedures may read from any module variable, Fortran SIMPLE procedure from F2023 will allow that, but they are far from common at that point).While doing a data flow analysis is likely unavoidable at some point, it will not address cases where the procedure body is not available in the current compilation unit, and will be rather expansive to do.
Luckily, Fortran language specifications allow the compiler to deduce that a procedure call cannot access a variable in many cases (this mainly stems from the 15.5.2.14 restrictions about dummy argument, and the inability to capture variables that do not have the TARGET attribute).
MLIR provides the perfect interface to leverages that:
AliasAnalysis::getModRef(mlir::Operation*op, mlir::Value location). This interface allows telling whetheropmay reference or modify the memory "location".This patch extends
fir::AliasAnalysis::getModRefto deal with fir.call. The cost is reasonable: "number of arguments" * "average(memory SSA defining-op chain depth)".It is currently very conservative and will only apply Fortran rules if:
It then:
Currently, it is always replying "ModRef" (may be referenced or modified) or "NoModRef" (may nor be referenced neither modified). This could be refined in the future to reply "Ref" for the cases where the only access is made via "Intent(IN)" argument.
It also inherits a lot of "false positive cases" coming from alias analysis current limitations (e.g., any copy-in/out on an arguments will make it return "ModRef" because alias analysis currently does not handle hlfir.copy_in in the SSA chain). These will be improved with time.
@klausler, I am adding you as a reviewer for the Fortran test (not the implementation) because it is very important that I am getting the language specifications correct here.
Any
! CHECK: function_name -> variable_name#0 : ModReflines in the test are verifying that the optimizer considers that, in the FIR representation of the Fortran code right above,call function_name()may access/modify the variablevariable_name(from the scope of the call). IfNoModRefis used instead ofModRef, the optimizer considers the variable cannot be accessed/modified. Please flag any expectations where you disagree (especially badNoModRef, which would be bugs, while bad "ModRef" will only cause missing optimization opportunities).This will allow implementing "array = array_function()" optimization in a future patch.