[flang][NFCI] Stop tracking memory source after a load in a more explicit manner. #126156
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,7 +51,7 @@ static bool hasGlobalOpTargetAttr(mlir::Value v, fir::AddrOfOp op) { | |
| v, fir::GlobalOp::getTargetAttrName(globalOpName)); | ||
| } | ||
|
|
||
| static mlir::Value getOriginalDef(mlir::Value v) { | ||
| mlir::Operation *defOp; | ||
| bool breakFromLoop = false; | ||
| while (!breakFromLoop && (defOp = v.getDefiningOp())) { | ||
|
|
@@ -578,16 +578,6 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v, | |
| breakFromLoop = true; | ||
| }) | ||
| .Case<fir::LoadOp>([&](auto op) { | ||
| // If load is inside target and it points to mapped item, | ||
| // continue tracking. | ||
| Operation *loadMemrefOp = op.getMemref().getDefiningOp(); | ||
|
|
@@ -600,6 +590,40 @@ AliasAnalysis::Source AliasAnalysis::getSource(mlir::Value v, | |
| defOp = v.getDefiningOp(); | ||
| return; | ||
| } | ||
|
|
||
| // If we are loading a box reference, but following the data, | ||
| // we gather the attributes of the box to populate the source | ||
| // and stop tracking. | ||
| if (auto boxTy = mlir::dyn_cast<fir::BaseBoxType>(ty); | ||
| boxTy && followingData) { | ||
|
Comment on lines
+597
to
+598
Collaborator
There was a problem hiding this comment. Is a goal of this new condition to ensure cases like For now (before we extend for alloca), is this PR intended to be NFC?
Contributor
Author
There was a problem hiding this comment. It does do that but it is not really the intention. The functionality should remain vastly the same, so maybe NFCI. Before and after, the load of a |
||
|
|
||
| if (mlir::isa<fir::PointerType>(boxTy.getEleTy())) | ||
| attributes.set(Attribute::Pointer); | ||
|
|
||
| auto def = getOriginalDef(op.getMemref()); | ||
| if (auto addrOfOp = def.template getDefiningOp<fir::AddrOfOp>()) { | ||
| global = addrOfOp.getSymbol(); | ||
|
|
||
| if (hasGlobalOpTargetAttr(def, addrOfOp)) | ||
| attributes.set(Attribute::Target); | ||
|
|
||
| type = SourceKind::Global; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Aren't here other cases where the Target attribute should be set (local/dummy allocatables with TARGET attribute for instance)? Why not calling
Contributor
Author
There was a problem hiding this comment. The plan was to be doing this in
Contributor
Author
There was a problem hiding this comment. Though, I have added a test that shows that we are reading the target attribute properly. It's only when we add support for local boxes that the attribute needs to be extracted from |
||
|
|
||
| // TODO: Add support to fir.alloca and fir.allocmem | ||
| // if (auto allocOp = def.template getDefiningOp<fir::AllocaOp>()) { | ||
| // ... | ||
| // } | ||
|
|
||
| if (isDummyArgument(def)) { | ||
| defOp = nullptr; | ||
| v = def; | ||
| } | ||
|
|
||
| breakFromLoop = true; | ||
| return; | ||
|
Collaborator
There was a problem hiding this comment. I just noticed that we now return here without setting
Contributor
Author
There was a problem hiding this comment. No, it was not. Though, offline @jeanPerier has been keen on having
Contributor
There was a problem hiding this comment. Yes, I am afraid we will end-up duplicating a lot of logic to walk and gather information, and that people will update I think it would be more powerfull/generic to call For instance, if the memref is a POINTER, technically the loaded address is not a pointer, it is a TARGET. If we cared about OPTIONAL, this aspect would not propagate from the memref to the loaded address, but things like VOLATILE and ASYNCHRONOUS probably would. TARGET propagates (an allocatable component of a deriver type that is TARGET can be taken by a pointer).... INTENT propagates. I believe Fortran committee wants to introduce a way to specify data vs descriptor "constness" (since INTENT always apply to both), and whatever attribute they come up with would not propagate.
Contributor
Author
There was a problem hiding this comment. I would say, let's proceed with #117785 which is now approved. Then revisit this with getSource. |
||
| } | ||
|
|
||
| // No further tracking for addresses loaded from memory for now. | ||
| type = SourceKind::Indirect; | ||
| breakFromLoop = true; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| // RUN: fir-opt %s -pass-pipeline='builtin.module(func.func(test-fir-alias-analysis))' 2>&1 | FileCheck %s | ||
|
|
||
| // The test was obtained from | ||
| // bbc test.f90 -emit-fir | ||
| // module mod | ||
| // real, pointer :: p0 | ||
| // real, allocatable :: alloc | ||
| // real, allocatable, target :: t_alloc | ||
| // real, target :: t | ||
| // real :: v | ||
| // end module | ||
| // | ||
| // subroutine test(n) | ||
| // use mod | ||
| // integer :: n | ||
| // real r1 | ||
| // p0 => t_alloc | ||
| // v = alloc | ||
| // r1 = p0 | ||
| // end subroutine test | ||
|
|
||
| // Checking that aliasing can only happen with an entity with the target attribute | ||
| // | ||
| // CHECK-DAG: r1#0 <-> t_alloc#0: NoAlias | ||
| // CHECK-DAG: r1#0 <-> alloc#0: NoAlias | ||
| // CHECK-DAG: t_alloc#0 <-> alloc#0: NoAlias | ||
| // CHECK-DAG: r1#0 <-> p0.ptr#0: NoAlias | ||
| // CHECK-DAG: t_alloc#0 <-> p0.ptr#0: MayAlias | ||
| // CHECK-DAG: alloc#0 <-> p0.ptr#0: NoAlias | ||
|
|
||
| fir.global @_QMmodEalloc : !fir.box<!fir.heap<f32>> { | ||
| %0 = fir.zero_bits !fir.heap<f32> | ||
| %1 = fir.embox %0 : (!fir.heap<f32>) -> !fir.box<!fir.heap<f32>> | ||
| fir.has_value %1 : !fir.box<!fir.heap<f32>> | ||
| } | ||
| fir.global @_QMmodEp0 : !fir.box<!fir.ptr<f32>> { | ||
| %0 = fir.zero_bits !fir.ptr<f32> | ||
| %1 = fir.embox %0 : (!fir.ptr<f32>) -> !fir.box<!fir.ptr<f32>> | ||
| fir.has_value %1 : !fir.box<!fir.ptr<f32>> | ||
| } | ||
| fir.global @_QMmodEt target : f32 { | ||
| %0 = fir.zero_bits f32 | ||
| fir.has_value %0 : f32 | ||
| } | ||
| fir.global @_QMmodEt_alloc target : !fir.box<!fir.heap<f32>> { | ||
| %0 = fir.zero_bits !fir.heap<f32> | ||
| %1 = fir.embox %0 : (!fir.heap<f32>) -> !fir.box<!fir.heap<f32>> | ||
| fir.has_value %1 : !fir.box<!fir.heap<f32>> | ||
| } | ||
| fir.global @_QMmodEv : f32 { | ||
| %0 = fir.zero_bits f32 | ||
| fir.has_value %0 : f32 | ||
| } | ||
| func.func @_QPtest(%arg0: !fir.ref<i32> {fir.bindc_name = "n"}) { | ||
| %0 = fir.dummy_scope : !fir.dscope | ||
| %1 = fir.address_of(@_QMmodEalloc) : !fir.ref<!fir.box<!fir.heap<f32>>> | ||
| %2 = fir.declare %1 {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = "_QMmodEalloc"} : (!fir.ref<!fir.box<!fir.heap<f32>>>) -> !fir.ref<!fir.box<!fir.heap<f32>>> | ||
| %3 = fir.declare %arg0 dummy_scope %0 {uniq_name = "_QFtestEn"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32> | ||
| %4 = fir.address_of(@_QMmodEp0) : !fir.ref<!fir.box<!fir.ptr<f32>>> | ||
| %5 = fir.declare %4 {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QMmodEp0"} : (!fir.ref<!fir.box<!fir.ptr<f32>>>) -> !fir.ref<!fir.box<!fir.ptr<f32>>> | ||
| %6 = fir.alloca f32 {bindc_name = "r1", uniq_name = "_QFtestEr1"} | ||
| %7 = fir.declare %6 {test.ptr="r1", uniq_name = "_QFtestEr1"} : (!fir.ref<f32>) -> !fir.ref<f32> | ||
| %8 = fir.address_of(@_QMmodEt) : !fir.ref<f32> | ||
| %9 = fir.declare %8 {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QMmodEt"} : (!fir.ref<f32>) -> !fir.ref<f32> | ||
| %10 = fir.address_of(@_QMmodEt_alloc) : !fir.ref<!fir.box<!fir.heap<f32>>> | ||
| %11 = fir.declare %10 {fortran_attrs = #fir.var_attrs<allocatable, target>, uniq_name = "_QMmodEt_alloc"} : (!fir.ref<!fir.box<!fir.heap<f32>>>) -> !fir.ref<!fir.box<!fir.heap<f32>>> | ||
| %12 = fir.address_of(@_QMmodEv) : !fir.ref<f32> | ||
| %13 = fir.declare %12 {uniq_name = "_QMmodEv"} : (!fir.ref<f32>) -> !fir.ref<f32> | ||
| %14 = fir.load %11 : !fir.ref<!fir.box<!fir.heap<f32>>> | ||
| %15 = fir.box_addr %14 {test.ptr="t_alloc"}: (!fir.box<!fir.heap<f32>>) -> !fir.heap<f32> | ||
| %16 = fir.embox %15 : (!fir.heap<f32>) -> !fir.box<!fir.ptr<f32>> | ||
| fir.store %16 to %5 : !fir.ref<!fir.box<!fir.ptr<f32>>> | ||
| %17 = fir.load %2 : !fir.ref<!fir.box<!fir.heap<f32>>> | ||
| %18 = fir.box_addr %17 {test.ptr="alloc"} : (!fir.box<!fir.heap<f32>>) -> !fir.heap<f32> | ||
| %19 = fir.load %18 : !fir.heap<f32> | ||
| fir.store %19 to %13 : !fir.ref<f32> | ||
| %20 = fir.load %5 : !fir.ref<!fir.box<!fir.ptr<f32>>> | ||
| %21 = fir.box_addr %20 {test.ptr="p0.ptr"} : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32> | ||
| %22 = fir.load %21 : !fir.ptr<f32> | ||
| fir.store %22 to %7 : !fir.ref<f32> | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea why the OpenMP case should not follow the same logic and continue the walk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why I added @DominikAdamski to the review. To show that we want to set a better precedent for loads and see if maybe something similar can be done for Omp. I did not quite follow the need for it.