Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
484cb90
WIP: [flang] AliasAnalysis: Fix pointer component logic
jdenny-ornl May 28, 2024
9f6d2a7
WIP: Fix case of ptr dummy arg vs. ptr component
jdenny-ornl May 30, 2024
78fc160
Fix logic in various ways, and add some tests
jdenny-ornl Jun 5, 2024
4273b38
Merge commit 'b3b9f8dd4cad' into flang-aa-ptr-component
jdenny-ornl Jun 11, 2024
492efb3
Check x%next vs. y%next
jdenny-ornl Jun 11, 2024
9b4d39a
Merge branch 'main' into flang-aa-ptr-component
jdenny-ornl Jun 13, 2024
ad0438c
Drop accidental -debug from fir-opt in test
jdenny-ornl Jun 13, 2024
1e5fdd4
Relocate some functions as requested by reviewer
jdenny-ornl Jun 13, 2024
3e48239
Value -> mlir::Value
jdenny-ornl Jun 19, 2024
bec0399
Fix and test HostAssoc case
jdenny-ornl Jun 19, 2024
776a17d
Fix and test after convert-hlfir-to-fir
jdenny-ornl Jun 20, 2024
09e91c6
Merge branch 'main' into flang-aa-ptr-component
jdenny-ornl Jul 10, 2024
4a73682
Encapsulate repeated logic into functions
jdenny-ornl Jul 12, 2024
ee248f5
Fix todos for handling addresses of pointers
jdenny-ornl Jul 12, 2024
a61be3a
Fix handling of dummy arg with target attribute
jdenny-ornl Jul 12, 2024
ea40a25
Give test more meaningful name
jdenny-ornl Jul 12, 2024
ba8e749
Merge branch 'main' into flang-aa-ptr-component
jdenny-ornl Aug 13, 2024
b14ff32
Merge branch 'main' into flang-aa-ptr-component
jdenny-ornl Aug 23, 2024
b31f352
Merge branch 'main' into flang-aa-ptr-component
jdenny-ornl Aug 26, 2024
553cf74
Remove todo about merging tests
jdenny-ornl Sep 5, 2024
285c2ce
Merge branch 'main' into flang-aa-ptr-component
jdenny-ornl Sep 5, 2024
27f1d94
Merge branch 'main' into flang-aa-ptr-component
jdenny-ornl Sep 16, 2024
aa4bb45
Merge branch 'main' into flang-aa-ptr-component
jdenny-ornl Sep 26, 2024
0766eba
Merge branch 'main' into flang-aa-ptr-component
jdenny-ornl Oct 14, 2024
942a625
Improve some function names and comments
jdenny-ornl Oct 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 19 additions & 11 deletions flang/lib/Optimizer/Analysis/AliasAnalysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -122,13 +122,32 @@ AliasResult AliasAnalysis::alias(Value lhs, Value rhs) {
}

if (lhsSrc.kind == rhsSrc.kind) {
// If the kinds and origins are the same, then lhs and rhs must alias unless
// either source is approximate. Approximate sources are for parts of the
// origin, but we don't have info here on which parts and whether they
// overlap, so we normally return MayAlias in that case.
//
// There is an exceptional case: The origins might compare unequal because
// only one has !isData(). If that source is approximate and the other is
// not, then the former is the source for the address *of* a pointer
// component in a composite, and the latter is for the address of that
// composite. As for the address of any composite vs. the address of one of
// its components, a store to one can affect a load from the other, so the
// result is MayAlias.
if (lhsSrc.origin == rhsSrc.origin) {
LLVM_DEBUG(llvm::dbgs()
<< " aliasing because same source kind and origin\n");
if (approximateSource)
return AliasResult::MayAlias;
return AliasResult::MustAlias;
}
if (lhsSrc.origin.u == rhsSrc.origin.u &&
((lhsSrc.approximateSource && !lhsSrc.isData() && !rhsSrc.approximateSource) ||
(rhsSrc.approximateSource && !rhsSrc.isData() && !lhsSrc.approximateSource))) {
LLVM_DEBUG(llvm::dbgs()
<< " aliasing between composite and pointer component\n");
return AliasResult::MayAlias;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Renaud-K posted the following comment as part of a commit, and I'm trying to move the conversation into this PR:

I don't think approximateSource is a reliable way of meaning component. We could have an optimized out fir.ccordinate_of %memref %c0, still referring to the same source.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Renaud-K I could extend Source with a field like bool component; It would be set to true for any DesignateOp specifying a component, and approximateSource would also still be set to true then. Does that seem like a reasonable direction to try?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DesignateOp is later lowered to coordinate_of for instance. The absence of which does not mean we are not dealing with a component. The alias analysis has to work at all levels. Again, fir.coordinate_of %memref %c0 is the same as %memref. We need to provide consistent results whether fir.coordinate_of is present or at least error on the conservative side.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DesignateOp is later lowered to coordinate_of for instance.

Thanks. In a simple example, I see that

%10 = hlfir.designate %1#0{"p"}   {fortran_attrs = #fir.var_attrs<pointer>} : (!fir.ref<!fir.type<_QMmTt{p:!fir.box<!fir.ptr<f32>>}>>) -> !fir.ref<!fir.box<!fir.ptr<f32>>>

becomes

%10 = fir.field_index p, !fir.type<_QMmTt{p:!fir.box<!fir.ptr<f32>>}>
%11 = fir.coordinate_of %1, %10 : (!fir.ref<!fir.type<_QMmTt{p:!fir.box<!fir.ptr<f32>>}>>, !fir.field) -> !fir.ref<!fir.box<!fir.ptr<f32>>>

The latter pattern seems simple enough to detect. I did not find a later version of that IR where fir.coordinate_of appeared but !fir.field and fir.field_index had been optimized away. Is your fir.coordinate_of %memref %c0 an example of that?

The absence of which does not mean we are not dealing with a component.

Is it true that either hlfir.designate or fir.coordinate_of must be present when accessing a component? If not, then, independently of this PR, we risk approximateSource=false and an invalid MustAlias, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MustAlias is correct if we can determine the source more accurately, and MayAlias is a conservative error. But in your case we could go from MayAlias (because we detect components) to NoAlias which is too optimistic. Maybe we can keep using the type, such as, if the type of the source is a derived type and the type of the starting point is not then we have a component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To finish the thought, I don't see why we have to give up (SourceKind=Indirect forces MayAlias) on tracking aliasing for a pointer just because it's a member of an alloca rather than the entire alloca.

Copy link
Contributor

@Renaud-K Renaud-K Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern would be to modify getOriginalDef to return anything other than Source::Indirect.
%11 with the designate case is always indirect. We have no idea what was stored in %9, it could a global, an allocatable or a dummy: we do not know.

There is room to improve on SourceKind::Indirect, such as using the type system to distinguish !fir.ptr from other memory references but that would be a different discussion. In the example just above, %11 would then be indirect too since we are following the data and not interested in the box address which happens to be an SourceKind::Alloca. Today, I would guess, we would get SourceKind::Alloca with an isData flag but it would be better modeled with SourceKind::Indirect and isData. Then we could do with SourceKind::Indirect what we used to do clumsily with SourceKind::Direct.

But I think, we should take it one step at a time. It would impact @tblah who is handling the (global, isData) separately. These two would become (indirect, isData)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern would be to modify getOriginalDef to return anything other than Source::Indirect.

For the record, getOriginalDef does not itself choose a SourceKind. Moreover, after getSource's LoadOp case calls getOriginalDef, getSource currently returns any of Indirect, Argument, or Global. That is true independently of this patch. No modification required.

(As you have noted, there are cases like alloca that getSource's LoadOp case doesn't currently fully handle. As a result, it currently returns Indirect sometimes where, for consistency with the current handling of dummy args and globals, it should return Allocate.)

%11 with the designate case is always indirect. We have no idea what was stored in %9, it could a global, an allocatable or a dummy: we do not know.

In that example, what was stored in %9 is the address of the pointer. In my above example without hlfir.designate, %3 is the address of the pointer. As far as I understand, the difference between those is whether that address is at a base address (for a global, dummy arg, alloca, etc.) or at an offset from that base address.

I don't see why that difference should affect the SourceKind for any value that is computed from the address of the pointer. In particular, if the source origin (i.e., %2) were a dummy arg or global, the SourceKind for %11 in my above example without hflir.designate would currently be Argument or Global, but the SourceKind for %11 in my above example with hlfir.designate would currently be Indirect. I see no reason for that difference. In a later patch, I would like to fix that. (Again, I would also like to add support for the alloca/Allocate case shown in the examples.)

There is room to improve on SourceKind::Indirect, such as using the type system to distinguish !fir.ptr from other memory references but that would be a different discussion. In the example just above, %11 would then be indirect too since we are following the data and not interested in the box address which happens to be an SourceKind::Alloca. Today, I would guess, we would get SourceKind::Alloca with an isData flag but it would be better modeled with SourceKind::Indirect and isData. Then we could do with SourceKind::Indirect what we used to do clumsily with SourceKind::Direct.

Whatever we decide to call the SourceKind for that case, do we agree that hlfir.designate should not change the SourceKind for %11 between my previous two examples?

But I think, we should take it one step at a time.

Agreed. Hopefully the current patch I'm trying to develop can be a next step.

It would impact @tblah who is handling the (global, isData) separately. These two would become (indirect, isData)

Where is the discussion of his work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdenny-ornl my work was proposed here https://discourse.llvm.org/t/rfc-propagate-fir-alias-analysis-information-using-tbaa/73755

There have since been changes since then to add support for Direct variables: 6242c8c

I think in this case the proposed changes won't alter the TBAA tags. My pass doesn't tag SourceKind::Alloca (by default) or SourceKind::Indirect (at all) (there are still TBAA tags added later during codegen to distinguish box metadata access from data access, but that is different again).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tblah Thanks for the pointers!


// Two host associated accesses may overlap due to an equivalence.
if (lhsSrc.kind == SourceKind::HostAssoc) {
Expand Down Expand Up @@ -173,17 +192,6 @@ AliasResult AliasAnalysis::alias(Value lhs, Value rhs) {
return AliasResult::MayAlias;
}

// Box for POINTER component inside an object of a derived type
// may alias box of a POINTER object, as well as boxes for POINTER
// components inside two objects of derived types may alias.
if ((src1->isRecordWithPointerComponent() && src2->isTargetOrPointer()) ||
(src2->isRecordWithPointerComponent() && src1->isTargetOrPointer()) ||
(src1->isRecordWithPointerComponent() &&
src2->isRecordWithPointerComponent())) {
LLVM_DEBUG(llvm::dbgs() << " aliasing because of pointer components\n");
return AliasResult::MayAlias;
}

return AliasResult::NoAlias;
}

Expand Down
19 changes: 9 additions & 10 deletions flang/test/Analysis/AliasAnalysis/alias-analysis-3.fir
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@
// end subroutine test
// end module m

// A composite with a pointer component may alias with a dummy pointer
// A composite with a pointer component does not alias with a dummy pointer
// CHECK-LABEL: Testing : "_QMmPtest
// CHECK: a#0 <-> func.region0#0: MayAlias
// CHECK: a#0 <-> func.region0#0: NoAlias

// FIXME: a's box cannot alias with raw reference to f32 (x), so MayAlias below must be NoAlias:
// CHECK: a#0 <-> func.region0#1: MayAlias
// a's box cannot alias with raw reference to f32 (x)
// CHECK: a#0 <-> func.region0#1: NoAlias

// pointer_dummy's box cannot alias with raw reference to f32 (x)
// CHECK: func.region0#0 <-> func.region0#1: NoAlias
Expand All @@ -46,7 +46,7 @@ func.func private @_QPtest2(!fir.ref<f32>)

// -----

// A composite with a pointer component may alias with a dummy
// A composite with a pointer component does not alias with a dummy
// argument of composite type with a pointer component:
// module m
// type t
Expand All @@ -63,7 +63,7 @@ func.func private @_QPtest2(!fir.ref<f32>)
// end module m

// CHECK-LABEL: Testing : "_QMmPtest"
// CHECK: a#0 <-> func.region0#0: MayAlias
// CHECK: a#0 <-> func.region0#0: NoAlias

fir.global @_QMmEa : !fir.type<_QMmTt{pointer_component:!fir.box<!fir.ptr<f32>>}> {
%0 = fir.undefined !fir.type<_QMmTt{pointer_component:!fir.box<!fir.ptr<f32>>}>
Expand All @@ -88,7 +88,7 @@ func.func private @_QPtest2(!fir.ref<f32>)
// -----

// Two dummy arguments of composite type with a pointer component
// may alias each other:
// do not alias each other:
// module m
// type t
// real, pointer :: pointer_component
Expand All @@ -103,7 +103,7 @@ func.func private @_QPtest2(!fir.ref<f32>)
// end module m

// CHECK-LABEL: Testing : "_QMmPtest"
// CHECK: func.region0#0 <-> func.region0#1: MayAlias
// CHECK: func.region0#0 <-> func.region0#1: NoAlias

func.func @_QMmPtest(%arg0: !fir.ref<!fir.type<_QMmTt{pointer_component:!fir.box<!fir.ptr<f32>>}>> {fir.bindc_name = "a"}, %arg1: !fir.ref<!fir.type<_QMmTt{pointer_component:!fir.box<!fir.ptr<f32>>}>> {fir.bindc_name = "b"}, %arg2: !fir.ref<f32> {fir.bindc_name = "x", fir.target}) attributes {test.ptr = "func"} {
%0 = fir.field_index pointer_component, !fir.type<_QMmTt{pointer_component:!fir.box<!fir.ptr<f32>>}>
Expand Down Expand Up @@ -137,8 +137,7 @@ func.func private @_QPtest2(!fir.ref<f32>)
// end module m

// CHECK-LABEL: Testing : "_QMmPtest"
// FIXME: MayAlias must be NoAlias
// CHECK: func.region0#0 <-> func.region0#1: MayAlias
// CHECK: func.region0#0 <-> func.region0#1: NoAlias

func.func @_QMmPtest(%arg0: !fir.ref<!fir.type<_QMmTt{allocatable_component:!fir.box<!fir.heap<f32>>}>> {fir.bindc_name = "a"}, %arg1: !fir.ref<!fir.type<_QMmTt{allocatable_component:!fir.box<!fir.heap<f32>>}>> {fir.bindc_name = "b"}) attributes {test.ptr = "func"} {
%0 = fir.field_index allocatable_component, !fir.type<_QMmTt{allocatable_component:!fir.box<!fir.heap<f32>>}>
Expand Down
12 changes: 6 additions & 6 deletions flang/test/Analysis/AliasAnalysis/alias-analysis-9.fir
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@
// end module

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check aliasing between x%next and and y%next?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing. I've added that check. It returns MayAlias. As the comment I added says, I think it should return NoAlias. We could try to fix that in this patch, but I think it would be better to address it in a separate patch because it involves a different part of the AliasAnalysis logic: the isTargetOrPointer() checks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed the cases I wanted covered here: #95287

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I plan to take a look today.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That part looks good to me. Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing. I've added that check. It returns MayAlias. As the comment I added says, I think it should return NoAlias. We could try to fix that in this patch, but I think it would be better to address it in a separate patch because it involves a different part of the AliasAnalysis logic: the isTargetOrPointer() checks.

I decided to go ahead and fix that in this PR as it helped me to work through another issue.

// CHECK-LABEL: Testing : "_QMmPfoo"
// TODO: x and y are non pointer, non target argument and therefore do not alias.
// CHECK-DAG: x#0 <-> y#0: MayAlias
// x and y are non pointer, non target argument and therefore do not alias.
// CHECK-DAG: x#0 <-> y#0: NoAlias

// TODO: y is not a pointer object and therefore does not alias with the x%next component.
// Also assigning x to y would not modify x.next
// CHECK-DAG: y#0 <-> xnext1#0: MayAlias
// CHECK-DAG: y#0 <-> xnext2#0: MayAlias
// y is not a pointer object and therefore does not alias with the x%next
// component. Also assigning x to y would not modify x.next
// CHECK-DAG: y#0 <-> xnext1#0: NoAlias
// CHECK-DAG: y#0 <-> xnext2#0: NoAlias

// We need to catch the fact that assigning y to x will modify xnext.
// The only side-effect between the 2 loads of x.next is the assignment to x,
Expand Down