Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
27 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
42382e0
Merge branch 'llvm:main' into fix-poly-copy-in-out
eugeneepshteyn Nov 3, 2025
5eff815
Begin the switch to ternary logic using std::optional<bool>. Renamed …
eugeneepshteyn Nov 3, 2025
68bab54
New name: ActualArgNeedsCopy(). Reworked the implicit interface case
eugeneepshteyn Nov 3, 2025
413718c
Continue refactoring. Extracted DummyNeedsContiguity() functionality …
eugeneepshteyn Nov 4, 2025
329445c
Continue refactoring
eugeneepshteyn Nov 4, 2025
ad080f1
clang-format
eugeneepshteyn Nov 4, 2025
8792ba4
Simplified check.HavePolymorphicDifferences(). Now seems to work
eugeneepshteyn Nov 4, 2025
8fd9427
clang-format
eugeneepshteyn Nov 4, 2025
4371a28
Merge branch 'llvm:main' into 1-copy-in-out-new-interface
eugeneepshteyn Nov 5, 2025
23393fe
Merge branch 'llvm:main' into fix-poly-copy-in-out
eugeneepshteyn Nov 5, 2025
1c202be
Merge branch '1-copy-in-out-new-interface' into fix-poly-copy-in-out
eugeneepshteyn Nov 5, 2025
3423413
Merge branch 'llvm:main' into fix-poly-copy-in-out
eugeneepshteyn Nov 10, 2025
80b2ccb
One code review comment (tests are still passing)
eugeneepshteyn Nov 6, 2025
2cef85a
Attempting to address Jean's comments. Currently 58 regressions in LI…
eugeneepshteyn Nov 7, 2025
d93b7e2
Fixed the regressions
eugeneepshteyn Nov 10, 2025
2abf0cb
Lower/force-temp.f90: added a test with IGNORE_TKR(C) and CONTIGUOUS
eugeneepshteyn Nov 10, 2025
57d67d7
Code review feedback
eugeneepshteyn Nov 10, 2025
e85c4e9
Merge branch 'main' into fix-poly-copy-in-out
eugeneepshteyn Nov 12, 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
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

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
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