-
Notifications
You must be signed in to change notification settings - Fork 12.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[flang] AliasAnalysis: distinguish addr of arg vs. addr in arg #87723
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
91def8c
[flang] AliasAnalysis: distinguish addr of arg vs. addr in arg
jdenny-ornl b4124cf
Fix case of allocatable arg
jdenny-ornl fd358af
Apply clang-format
jdenny-ornl 75e2a26
Improve dummy argument identification
jdenny-ornl ca7a18b
Rename a symbol in a test for consistency
jdenny-ornl 0af947a
Merge branch 'main' into flang-aa-load-arg
jdenny-ornl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
94 changes: 94 additions & 0 deletions
94
flang/test/Analysis/AliasAnalysis/alias-analysis-box-addr-load/arg.fir
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
// Check aliasing with the address passed via a pointer dummy argument. | ||
|
||
// Use --mlir-disable-threading so that the AA queries are serialized | ||
// as well as its diagnostic output. | ||
// RUN: fir-opt %s \ | ||
// RUN: -pass-pipeline='builtin.module(func.func(test-fir-alias-analysis))' \ | ||
// RUN: --mlir-disable-threading 2>&1 | FileCheck -match-full-lines %s | ||
|
||
// subroutine test(p0, p1, arr, t_arr, alloc, t_alloc) | ||
// real, pointer :: p0, p1 | ||
// real :: arr(:) | ||
// real, target :: t_arr(:) | ||
// real, allocatable :: alloc | ||
// real, allocatable, target :: t_alloc | ||
// real, target :: t | ||
// real :: v | ||
// v = p0 | ||
// v = p1 | ||
// v = arr(1) | ||
// v = t_arr(1) | ||
// v = alloc | ||
// v = t_alloc | ||
// end subroutine test | ||
|
||
// CHECK-LABEL: Testing : "_QPtest" | ||
|
||
// The address in a pointer can alias the address in another pointer or the | ||
// address of a target but not the address of other variables. | ||
// CHECK-DAG: t.addr#0 <-> p0.tgt_addr#0: MayAlias | ||
// CHECK-DAG: t.addr#0 <-> p1.tgt_addr#0: MayAlias | ||
// CHECK-DAG: v.addr#0 <-> p0.tgt_addr#0: NoAlias | ||
// CHECK-DAG: v.addr#0 <-> p1.tgt_addr#0: NoAlias | ||
// CHECK-DAG: p0.tgt_addr#0 <-> p1.tgt_addr#0: MayAlias | ||
|
||
// Determining whether the address *in* a pointer can alias the address *of* a | ||
// pointer is not yet handled. In the past, when it was the same pointer, that | ||
// relationship was mistakenly determined to be MustAlias. | ||
// CHECK-DAG: p0.tgt_addr#0 <-> func.region0#0: MayAlias | ||
// CHECK-DAG: p0.tgt_addr#0 <-> func.region0#1: MayAlias | ||
// CHECK-DAG: p1.tgt_addr#0 <-> func.region0#0: MayAlias | ||
// CHECK-DAG: p1.tgt_addr#0 <-> func.region0#1: MayAlias | ||
|
||
// For some cases, AliasAnalysis analyzes hlfir.designate like fir.box_addr, so | ||
// make sure it doesn't mistakenly see arr(1).addr as an address that was loaded | ||
// from a pointer and that could alias something. However, t_arr is a target. | ||
// CHECK-DAG: p0.tgt_addr#0 <-> arr(1).addr#0: NoAlias | ||
// CHECK-DAG: p0.tgt_addr#0 <-> t_arr(1).addr#0: MayAlias | ||
|
||
// Like a pointer, an allocatable contains an address, but an allocatable is not | ||
// a pointer and so cannot alias pointers. However, t_alloc is a target. | ||
// CHECK-DAG: p0.tgt_addr#0 <-> alloc.tgt_addr#0: NoAlias | ||
// CHECK-DAG: p0.tgt_addr#0 <-> t_alloc.tgt_addr#0: MayAlias | ||
|
||
// The address *in* an allocatable cannot alias the address *of* that | ||
// allocatable. | ||
// CHECK-DAG: alloc.addr#0 <-> alloc.tgt_addr#0: NoAlias | ||
|
||
func.func @_QPtest(%arg0: !fir.ref<!fir.box<!fir.ptr<f32>>> {fir.bindc_name = "p0"}, %arg1: !fir.ref<!fir.box<!fir.ptr<f32>>> {fir.bindc_name = "p1"}, %arg2: !fir.box<!fir.array<?xf32>> {fir.bindc_name = "arr"}, %arg3: !fir.box<!fir.array<?xf32>> {fir.bindc_name = "t_arr", fir.target}, %arg4: !fir.ref<!fir.box<!fir.heap<f32>>> {fir.bindc_name = "alloc"}, %arg5: !fir.ref<!fir.box<!fir.heap<f32>>> {fir.bindc_name = "t_alloc", fir.target}) attributes {test.ptr="func"} { | ||
%0:2 = hlfir.declare %arg4 {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = "_QFtestEalloc", test.ptr="alloc.addr"} : (!fir.ref<!fir.box<!fir.heap<f32>>>) -> (!fir.ref<!fir.box<!fir.heap<f32>>>, !fir.ref<!fir.box<!fir.heap<f32>>>) | ||
%1:2 = hlfir.declare %arg2 {uniq_name = "_QFtestEarr"} : (!fir.box<!fir.array<?xf32>>) -> (!fir.box<!fir.array<?xf32>>, !fir.box<!fir.array<?xf32>>) | ||
%2:2 = hlfir.declare %arg0 {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFtestEp0"} : (!fir.ref<!fir.box<!fir.ptr<f32>>>) -> (!fir.ref<!fir.box<!fir.ptr<f32>>>, !fir.ref<!fir.box<!fir.ptr<f32>>>) | ||
%3:2 = hlfir.declare %arg1 {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFtestEp1"} : (!fir.ref<!fir.box<!fir.ptr<f32>>>) -> (!fir.ref<!fir.box<!fir.ptr<f32>>>, !fir.ref<!fir.box<!fir.ptr<f32>>>) | ||
%4 = fir.alloca f32 {bindc_name = "t", fir.target, uniq_name = "_QFtestEt"} | ||
%5:2 = hlfir.declare %4 {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFtestEt", test.ptr="t.addr"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>) | ||
%6:2 = hlfir.declare %arg5 {fortran_attrs = #fir.var_attrs<allocatable, target>, uniq_name = "_QFtestEt_alloc"} : (!fir.ref<!fir.box<!fir.heap<f32>>>) -> (!fir.ref<!fir.box<!fir.heap<f32>>>, !fir.ref<!fir.box<!fir.heap<f32>>>) | ||
%7:2 = hlfir.declare %arg3 {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFtestEt_arr"} : (!fir.box<!fir.array<?xf32>>) -> (!fir.box<!fir.array<?xf32>>, !fir.box<!fir.array<?xf32>>) | ||
%8 = fir.alloca f32 {bindc_name = "v", uniq_name = "_QFtestEv"} | ||
%9:2 = hlfir.declare %8 {uniq_name = "_QFtestEv", test.ptr="v.addr"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>) | ||
%10 = fir.load %2#0 : !fir.ref<!fir.box<!fir.ptr<f32>>> | ||
%11 = fir.box_addr %10 {test.ptr="p0.tgt_addr"} : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32> | ||
%12 = fir.load %11 : !fir.ptr<f32> | ||
hlfir.assign %12 to %9#0 : f32, !fir.ref<f32> | ||
%13 = fir.load %3#0 : !fir.ref<!fir.box<!fir.ptr<f32>>> | ||
%14 = fir.box_addr %13 {test.ptr="p1.tgt_addr"} : (!fir.box<!fir.ptr<f32>>) -> !fir.ptr<f32> | ||
%15 = fir.load %14 : !fir.ptr<f32> | ||
hlfir.assign %15 to %9#0 : f32, !fir.ref<f32> | ||
%c1 = arith.constant 1 : index | ||
%16 = hlfir.designate %1#0 (%c1) {test.ptr="arr(1).addr"} : (!fir.box<!fir.array<?xf32>>, index) -> !fir.ref<f32> | ||
%17 = fir.load %16 : !fir.ref<f32> | ||
hlfir.assign %17 to %9#0 : f32, !fir.ref<f32> | ||
%c1_0 = arith.constant 1 : index | ||
%18 = hlfir.designate %7#0 (%c1_0) {test.ptr="t_arr(1).addr"} : (!fir.box<!fir.array<?xf32>>, index) -> !fir.ref<f32> | ||
%19 = fir.load %18 : !fir.ref<f32> | ||
hlfir.assign %19 to %9#0 : f32, !fir.ref<f32> | ||
%20 = fir.load %0#0 : !fir.ref<!fir.box<!fir.heap<f32>>> | ||
%21 = fir.box_addr %20 {test.ptr="alloc.tgt_addr"} : (!fir.box<!fir.heap<f32>>) -> !fir.heap<f32> | ||
%22 = fir.load %21 : !fir.heap<f32> | ||
hlfir.assign %22 to %9#0 : f32, !fir.ref<f32> | ||
%23 = fir.load %6#0 : !fir.ref<!fir.box<!fir.heap<f32>>> | ||
%24 = fir.box_addr %23 {test.ptr="t_alloc.tgt_addr"} : (!fir.box<!fir.heap<f32>>) -> !fir.heap<f32> | ||
%25 = fir.load %24 : !fir.heap<f32> | ||
hlfir.assign %25 to %9#0 : f32, !fir.ref<f32> | ||
return | ||
} |
This file contains 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
Oops, something went wrong.
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.
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.
I believe, this might be an attempt at handling INTENT(IN). With INTENT(IN) the box is passed by value so there is no !fir.ref type. Could we check for that instead?
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.
I'm not sure I understand your meaning. Given the following:
flang-new generates:
If I drop the
intent(in)
, the only difference is that theintent_in
attribute is lost.Would you please provide an example of the case you have in mind? Thanks.
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.
If I remove the
fir::isa_ref_type(ty)
check, several tests fail. For example, in the functiontest3
inflang/test/Analysis/AliasAnalysis/alias-analysis-host-assoc.fir
,y(1)
is then analyzed as aDirect
instead of anArgument
. It has type!fir.box<!fir.array<?xi32>>
and is declared asinteger, target :: y(:)
. Is that the information you're looking for?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.
After further thought, maybe there is a cleaner way to write this check:
followBoxAddrLoad
variable local toAliasAnalysis::getSource
, and set it to true right before thereturn
in thefir::LoadOp
case.followBoxAddr && fir::isa_ref_type(ty)
withfollowBoxAddrLoad
.fir::AddrOfOp
check, replacefollowBoxAddr && mlir::isa<fir::BaseBoxType>(fir::unwrapRefType(ty))
withfollowBoxAddrLoad
.I tried that, and check-flang still succeeded. What do you think?
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.
I am in the process of compiling your changes and will give them a better look today. What you describe though sounds really good. I will give it a shot.
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.
Right, when I wrote that comment, I somehow forgot that I generalized beyond pointers. See my previous comment.
While the cases I'm trying to address so far specifically involve the
fir::LoadOp
case inAliasAnalysis::getSource
, I'd appreciate any fortran example that does not. My understanding of relevant use cases is probably too narrow. [edit: Oops, you just posted one while I was writing this.]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.
I wonder if it will be more clear to only keep the first four source kinds and move
Direct
into the attributes. We are looking for the source entity here, so the source kind should represent this source entity. Whatever happens during the use-def chain walk should not affect the kind of the source entity that we end up finding. TheDirect
, in this case, would mean that the reference value passed togetSource
is a result of one or more dereferences of the original source. I just find it easier to think about it this way, though, you may know cases where it can complicate the implementation.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.
Thanks for the example. That helps. However, I'm struggling with the idea of performing a FIR alias analysis on something (the box in this example) that FIR represents as a value not an address. If indeed it never makes sense to do that, then it seems it wouldn't matter whether we distinguish this box from its content as only the content is a candidate for alias analysis.
Might make sense. However, I've been assuming that, once you extract an address from a box, then you no longer have a global, dummy arg, local, or whatever the box is, so I thought it was ok to just call it a
Direct
. Maybe that's wrong? On that point, does anyone object to this patch's changes toflang/test/Transforms/tbaa2.fir
? There, this patch causes the address extracted from a dummy argument allocatable to be labeledSourceKind::Direct
and notSourceKind::Argument
. Is that problematic?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.
Right, we could do some ground work before adding further enhancements to enforce that we start from a memory reference always. I wanted to check with @jeanPerier before we do that, in case he might need to make adjustments in lowering.
Let's note that when we are at the LLVM level, the box is an address and it becomes a legitimate question to ask. I will have to dig some to see how we handle this case.
What I would be struggling with is that following the address of a POINTER takes you to a SourceKind::Direct but following the address of a TARGET takes you to a SourceKind::Argument.
Yes, then we would get consistent source kinds for target and pointers and we should get the same attribute, that I would hope we would have renamed to something other than "Direct" then. That would require some buy-in from @tblah as it would require changes in
runOnAliasInterface
though it should not cause any change in the results.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.
Talking to Jean, reminded me that we are already handling the query on a box value as a query on the data.
bool followBoxAddr{mlir::isa<fir::BaseBoxType>(v.getType())};
We initialize
followBoxAddr
to true if we are starting from a box.Though this is a moot point for this discussion since you are interested on a distinction between references.
I am working on a proposal to eliminate SourceKind::Direct while still retaining the information it was capturing. You still will be able to distinguish between data and box. I will write up an RFC tomorrow and will send out a WIP PR.