Skip to content

[flang] Fixed regression in copy-in/copy-out #161259

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

[flang] Fixed regression in copy-in/copy-out #161259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
632ed7b
[flang] Fixed regression in copy-in/copy-out
eugeneepshteyn Sep 29, 2025
97cbeed
Merge branch 'llvm:main' into fix-poly-copy-in-out
eugeneepshteyn Sep 30, 2025
9189ede
Restore HavePolymorphicDifferences(), but with different checks
eugeneepshteyn Sep 30, 2025
f572da2
Tweaked the change to ignore assumed type in polymorphic check
eugeneepshteyn Sep 30, 2025
91f9e67
clang-format
eugeneepshteyn Sep 30, 2025
f52136b
Merge branch 'main' into fix-poly-copy-in-out
eugeneepshteyn Sep 30, 2025
d60edf8
Replaced s6() with an interface, since only care about the call
eugeneepshteyn Sep 30, 2025
1e69803
New test to check for copy-in/out for potential slicing test
eugeneepshteyn Sep 30, 2025
94f69ed
Tweaked test comment
eugeneepshteyn Sep 30, 2025
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
39 changes: 14 additions & 25 deletions flang/lib/Evaluate/check-expression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1493,32 +1493,21 @@ class CopyInOutExplicitInterface {
return !actualTreatAsContiguous && dummyNeedsContiguity;
}

// Returns true, if actual and dummy have polymorphic differences
bool HavePolymorphicDifferences() const {
bool dummyIsAssumedRank{dummyObj_.type.attrs().test(
characteristics::TypeAndShape::Attr::AssumedRank)};
bool actualIsAssumedRank{semantics::IsAssumedRank(actual_)};
bool dummyIsAssumedShape{dummyObj_.type.attrs().test(
characteristics::TypeAndShape::Attr::AssumedShape)};
bool actualIsAssumedShape{semantics::IsAssumedShape(actual_)};
if ((actualIsAssumedRank && dummyIsAssumedRank) ||
(actualIsAssumedShape && dummyIsAssumedShape)) {
// Assumed-rank and assumed-shape arrays are represented by descriptors,
// so don't need to do polymorphic check.
} else if (!dummyObj_.ignoreTKR.test(common::IgnoreTKR::Type)) {
// flang supports limited cases of passing polymorphic to non-polimorphic.
// These cases require temporary of non-polymorphic type. (For example,
// the actual argument could be polymorphic array of child type,
// while the dummy argument could be non-polymorphic array of parent
// type.)
bool dummyIsPolymorphic{dummyObj_.type.type().IsPolymorphic()};
auto actualType{
characteristics::TypeAndShape::Characterize(actual_, fc_)};
bool actualIsPolymorphic{
actualType && actualType->type().IsPolymorphic()};
if (actualIsPolymorphic && !dummyIsPolymorphic) {
return true;
}
// These cases require temporary of non-polymorphic type. (For example,
// the actual argument could be polymorphic array of child type,
Copy link
Contributor

Choose a reason for hiding this comment

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

An actual argument that is class(parent) would require a temporary for a type(parent) dummy; type extension isn't necessary.

An actual argument that has a monomorphic type that is an extension of a type(t) dummy will require a temporary unless it actually has no additional components or TBP overrides relative to t.

// while the dummy argument could be non-polymorphic array of parent
// type.)
if (dummyObj_.ignoreTKR.test(common::IgnoreTKR::Type)) {
return false;
}
auto actualType{characteristics::TypeAndShape::Characterize(actual_, fc_)};
if (actualType && actualType->type().IsPolymorphic() &&
!actualType->type().IsAssumedType() &&
!dummyObj_.IsPassedByDescriptor(/*isBindC*/ false)) {
Copy link
Contributor

@jeanPerier jeanPerier Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I do not get why passing by descriptor matters here. It is possible to have assumed shape arrays with the CONTIGUOUS attribute in which case a contiguous copy is still needed even if they are passed by descriptors.

To me, there is a lot of complexity in trying to get an accurate answer at compile time, while outside of the IGNORE_TKR case, it is just fine to generate too much copy-in/copy-out instructions since there is a runtime contiguity check and the copy is not done when not needed. Outside of the IGNORE_TKR case, we could just always generate them for explicit shape dummy arguments and assumed-shape with CONTIGUOUS attribute and get valid code even in the cases where making a copy is invalid.

I understand the idea of trying to detect as much as possible situations where you know an actual copy-in/copy-out will happen and to warn/error for cases with VOLATILE and al, but I think using the same logic in lowering comes at a risk of false negative where a copy-in/copy-out could be missed and it is unclear to me how good is our test coverage here, especially with polymorphic arguments (missing copy-in/copy-out code can easily go undetected if the actual happens to be contiguous).

The problem I see with sharing the logic between lowering and semantics is that you now need to get both side of the answer prefect. Too much true and flang will raise bogus errors regarding C1547, too much false and flang will miss generation of copy-in/copy-out like in the current bug.

To avoid having to come code that accurately deals with every situation, I would suggest using a ternary logic. std::optional, where std::nullopt means not sure, or just revert lowering to use its own logic that will generate more copy-in/copy-out instruction than probably needed, but that I found easier to audit (fixing the IGNORE_TKR case there. As far as I can tell the only issue in lowering was that it was missing the handling of IGNORE_TKR (C) in PassedEntity::mustBeMadeContiguous).

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to the use of std::optional<bool> here; it's what IsContiguous() returns for similar reasons.

I would like us to try to have a single implementation here for both semantics and lowering, if one can be found; it would be hard to have two implementations and be assured that they were identical.

It should be possible to write more tests that can verify that code that is warning-free at compilation time doesn't use copy-in at run-time (use LOC), and that code that does use copy-in at run-time got warnings at compilation time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not get why passing by descriptor matters here. It is possible to have assumed shape arrays with the CONTIGUOUS attribute in which case a contiguous copy is still needed even if they are passed by descriptors.

That's why contiguity check is done first. Once contiguity is out of the picture, checking for descriptor differences between actual and dummy args seems to work nicely here.

// Not passing a descriptor, so will need to make a copy of the data
// with a proper type.
return true;
}
return false;
}
Expand Down
43 changes: 43 additions & 0 deletions flang/test/Lower/force-temp.f90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@ subroutine pass_intent_out(buf)
integer, intent(out) :: buf(5)
end subroutine
end interface

! Used by call_s6() and others below
type base
integer :: i = -1
end type
type, extends (base) :: child
real :: r = -2.0
end type
contains
subroutine s1(buf)
!CHECK-LABEL: func.func @_QMtestPs1
Expand Down Expand Up @@ -79,4 +87,39 @@ subroutine s5()
p => x(::2) ! pointer to non-contiguous array section
call pass_intent_out(p)
end subroutine
subroutine call_s6()
interface
subroutine s6(b)
import :: base
type(base), intent(inout) :: b(:)
end subroutine s6
end interface
class(base), pointer :: pb(:)
type(child), target :: c(2)
!CHECK-LABEL: func.func @_QMtestPcall_s6
!CHECK-NOT: hlfir.copy_in
!CHECK: fir.call @_QPs6
!CHECK-NOT: hlfir.copy_out
pb => c
call s6(pb)
end subroutine call_s6
subroutine call_s7()
interface
subroutine s7(b1, b2, n)
import :: base
integer :: n
type(base), intent(inout) :: b1(n)
type(base), intent(inout) :: b2(*)
end subroutine
end interface
integer, parameter :: n = 7
class(base), allocatable :: c1(:), c2(:)
!CHECK-LABEL: func.func @_QMtestPcall_s7
!CHECK: hlfir.copy_in
!CHECK: hlfir.copy_in
!CHECK: fir.call @_QPs7
!CHECK: hlfir.copy_out
!CHECK: hlfir.copy_out
call s7(c1, c2, n)
end subroutine call_s7
end module