-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[flang] Create temporaries for array sections passed to IGNORE_TKR dummy args #147419
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
Changes from all commits
2477076
50dd216
383bdca
1e76690
28cb65f
a176d2d
96daba4
aace0c2
5fdfb45
d105d0d
78fb202
bd1e8e6
a093fe6
63b2811
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1416,16 +1416,26 @@ bool Fortran::lower::CallInterface<T>::PassedEntity::isIntentOut() const { | |
| return true; | ||
| return characteristics->GetIntent() == Fortran::common::Intent::Out; | ||
| } | ||
|
|
||
| /// Returning "true" from this function is a prerequisite for running | ||
| /// contiguity check on the actual argument. | ||
| template <typename T> | ||
| bool Fortran::lower::CallInterface<T>::PassedEntity::mustBeMadeContiguous() | ||
| const { | ||
| bool Fortran::lower::CallInterface<T>::PassedEntity::mustBeMadeContiguous( | ||
| const bool argHasTriplet) const { | ||
| if (!characteristics) | ||
| return true; | ||
| const auto *dummy = | ||
| std::get_if<Fortran::evaluate::characteristics::DummyDataObject>( | ||
| &characteristics->u); | ||
| if (!dummy) | ||
| return false; | ||
| if (dummy->ignoreTKR.test(common::IgnoreTKR::Contiguous)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change makes sense to me, but I am not sure about the code below. |
||
| return false; | ||
|
|
||
| // TODO: should this check ignore "device" or "managed"? | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reviewers, please answer this question |
||
| if (dummy->ignoreTKR.any() && argHasTriplet) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we care about whether the actual argument is an array section or not? Can it be a non-contiguous array (e.g. assumed shape dummy) that is passed as a dummy that is declared as To fix #138471, can we return the following at line 1448? |
||
| return true; | ||
|
|
||
| const auto &shapeAttrs = dummy->type.attrs(); | ||
| using ShapeAttrs = Fortran::evaluate::characteristics::TypeAndShape::Attr; | ||
| if (shapeAttrs.test(ShapeAttrs::AssumedRank) || | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| ! RUN: bbc -emit-hlfir -o - %s | FileCheck %s | ||
| ! Ensure that we still create copy_in/copy_out for non-contiguous input, | ||
| ! despite having IGNORE_TKR. | ||
| ! | ||
| module test | ||
| implicit none(type, external) | ||
| contains | ||
| subroutine pass_ignore_tkr(buf, n) | ||
| implicit none | ||
| !DIR$ IGNORE_TKR buf | ||
| real, intent(inout) :: buf(n) | ||
| integer, intent(in) :: n | ||
| end subroutine | ||
|
|
||
| subroutine pass_ignore_tkr_c(buf, n) | ||
| implicit none | ||
| !DIR$ IGNORE_TKR (tkrc) buf | ||
| real, intent(inout) :: buf(n) | ||
| integer, intent(in) :: n | ||
| end subroutine | ||
|
|
||
| subroutine s1() | ||
| !CHECK-LABEL: func.func @_QMtestPs1() | ||
| !CHECK: hlfir.copy_in | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this test fail withtout your change? It seems that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what happens, when one oversimplifies the test. I will upload corrected test tomorrow. |
||
| !CHECK: fir.call @_QMtestPpass_ignore_tkr | ||
| !CHECK: hlfir.copy_out | ||
|
|
||
| integer :: x(5) | ||
| x = [1,2,3,4,5] | ||
| ! Non-contiguous input | ||
| call pass_ignore_tkr(x(1::2), size(x(1::2))) | ||
| end subroutine s1 | ||
|
|
||
| subroutine s2() | ||
| !CHECK-LABEL: func.func @_QMtestPs2() | ||
| !CHECK-NOT: hlfir.copy_in | ||
| !CHECK: fir.call @_QMtestPpass_ignore_tkr | ||
| !CHECK-NOT: hlfir.copy_out | ||
|
|
||
| integer :: x(5) | ||
| x = [1,2,3,4,5] | ||
| ! Contiguous input | ||
| call pass_ignore_tkr(x(1:3), size(x(1:3))) | ||
| end subroutine s2 | ||
|
|
||
| subroutine s3() | ||
| !CHECK-LABEL: func.func @_QMtestPs3() | ||
| !CHECK-NOT: hlfir.copy_in | ||
| !CHECK: fir.call @_QMtestPpass_ignore_tkr_c | ||
| !CHECK-NOT: hlfir.copy_out | ||
|
|
||
| integer :: x(5) | ||
| x = [1,2,3,4,5] | ||
| ! Non-contiguous input, but the dummy arg declaration ignores | ||
| ! the contiguity check | ||
| call pass_ignore_tkr_c(x(1::2), size(x(1::2))) | ||
| end subroutine s3 | ||
| end module test | ||
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.
@klausler , could you please review
HasTripletpart? I'll add the other reviewers for lowering, once I add the test.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.
Simply checking for triplets is going to give false positives.
A(:)is contiguous whenAis; so areA(J:K)andA(J:K:1). There's a general contiguity checking facility in Evaluate that can handle questions of contiguity; have you tried using it?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
mustBeMadeContiguous()returns true, then this eventually gets toisSimplyContiguous()which is a helper function that callsFortran::evaluate::IsSimplyContiguous().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.
And if
mustBeMadeContiguousreturns false, thenIsSimplyContiguousis not called? Won't that lead to false negatives from e.g.A%X, non-CONTIGUOUS pointers, &c.?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.
That would also be a problem with the current source, without my modifications. I'll check what happens in the cases you mentioned.