[flang] Fixed regression in copy-in/copy-out #161259
Conversation
Removed incorrect polymprphic check, added regression test. Fixes llvm#159149
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-semantics Author: Eugene Epshteyn (eugeneepshteyn) ChangesRemoved incorrect polymprphic check, added regression test. Fixes #159149 Full diff: https://github.com/llvm/llvm-project/pull/161259.diff 2 Files Affected:
diff --git a/flang/lib/Evaluate/check-expression.cpp b/flang/lib/Evaluate/check-expression.cpp
index 8931cbe485ac2..b35fff70cabaf 100644
--- a/flang/lib/Evaluate/check-expression.cpp
+++ b/flang/lib/Evaluate/check-expression.cpp
@@ -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,
+ // 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)) {
+ // Not passing a descriptor, so will need to make a copy of the data
+ // with a proper type.
+ return true;
}
return false;
}
diff --git a/flang/test/Lower/force-temp.f90 b/flang/test/Lower/force-temp.f90
index d9ba543d46313..e02463f700b45 100644
--- a/flang/test/Lower/force-temp.f90
+++ b/flang/test/Lower/force-temp.f90
@@ -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
@@ -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
|
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Eugene Epshteyn (eugeneepshteyn) ChangesRemoved incorrect polymprphic check, added regression test. Fixes #159149 Full diff: https://github.com/llvm/llvm-project/pull/161259.diff 2 Files Affected:
diff --git a/flang/lib/Evaluate/check-expression.cpp b/flang/lib/Evaluate/check-expression.cpp
index 8931cbe485ac2..b35fff70cabaf 100644
--- a/flang/lib/Evaluate/check-expression.cpp
+++ b/flang/lib/Evaluate/check-expression.cpp
@@ -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,
+ // 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)) {
+ // Not passing a descriptor, so will need to make a copy of the data
+ // with a proper type.
+ return true;
}
return false;
}
diff --git a/flang/test/Lower/force-temp.f90 b/flang/test/Lower/force-temp.f90
index d9ba543d46313..e02463f700b45 100644
--- a/flang/test/Lower/force-temp.f90
+++ b/flang/test/Lower/force-temp.f90
@@ -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
@@ -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
|
| return true; | ||
| } | ||
| // These cases require temporary of non-polymorphic type. (For example, | ||
| // the actual argument could be polymorphic array of child type, |
There was a problem hiding this comment.
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.
| auto actualType{characteristics::TypeAndShape::Characterize(actual_, fc_)}; | ||
| if (actualType && actualType->type().IsPolymorphic() && | ||
| !actualType->type().IsAssumedType() && | ||
| !dummyObj_.IsPassedByDescriptor(/*isBindC*/ false)) { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
+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.
There was a problem hiding this comment.
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.
4 participants
Fixed the polymprphic check for copy-in/copy-out, added regression tests.
Fixes #159149