Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions flang/include/flang/Evaluate/tools.h
Original file line number Diff line number Diff line change
Expand Up @@ -1123,6 +1123,9 @@ extern template semantics::UnorderedSymbolSet CollectCudaSymbols(
// Predicate: does a variable contain a vector-valued subscript (not a triplet)?
bool HasVectorSubscript(const Expr<SomeType> &);

// Predicate: does a variable contain a triplet?
bool HasTriplet(const Expr<SomeType> &);

// Predicate: does an expression contain constant?
bool HasConstant(const Expr<SomeType> &);

Expand Down
2 changes: 1 addition & 1 deletion flang/include/flang/Lower/CallInterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ class CallInterface {
/// Is the argument INTENT(OUT)
bool isIntentOut() const;
/// Does the argument have the CONTIGUOUS attribute or have explicit shape?
bool mustBeMadeContiguous() const;
bool mustBeMadeContiguous(bool argHasTriplet = false) const;
/// Does the dummy argument have the VALUE attribute?
bool hasValueAttribute() const;
/// Does the dummy argument have the ALLOCATABLE attribute?
Expand Down
16 changes: 16 additions & 0 deletions flang/lib/Evaluate/tools.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1173,6 +1173,22 @@ bool HasVectorSubscript(const Expr<SomeType> &expr) {
return HasVectorSubscriptHelper{}(expr);
}

// HasTriplet()
Copy link
Contributor Author

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 HasTriplet part? I'll add the other reviewers for lowering, once I add the test.

Copy link
Contributor

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 when A is; so are A(J:K) and A(J:K:1). There's a general contiguity checking facility in Evaluate that can handle questions of contiguity; have you tried using it?

Copy link
Contributor Author

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 to isSimplyContiguous() which is a helper function that calls Fortran::evaluate::IsSimplyContiguous().

Copy link
Contributor

Choose a reason for hiding this comment

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

And if mustBeMadeContiguous returns false, then IsSimplyContiguous is not called? Won't that lead to false negatives from e.g. A%X, non-CONTIGUOUS pointers, &c.?

Copy link
Contributor Author

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.

struct HasTripletHelper : public AnyTraverse<HasTripletHelper, bool,
/*TraverseAssocEntityDetails=*/false> {
using Base = AnyTraverse<HasTripletHelper, bool, false>;
HasTripletHelper() : Base{*this} {}
using Base::operator();
bool operator()(const Subscript &ss) const {
return std::holds_alternative<Triplet>(ss.u);
}
bool operator()(const ProcedureRef &) const {
return false; // don't descend into function call arguments
}
};

bool HasTriplet(const Expr<SomeType> &expr) { return HasTripletHelper{}(expr); }

// HasConstant()
struct HasConstantHelper : public AnyTraverse<HasConstantHelper, bool,
/*TraverseAssocEntityDetails=*/false> {
Expand Down
14 changes: 12 additions & 2 deletions flang/lib/Lower/CallInterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The 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"?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reviewers, please answer this question

if (dummy->ignoreTKR.any() && argHasTriplet)
Copy link
Contributor

Choose a reason for hiding this comment

The 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 IGNORE_TKR?

To fix #138471, can we return the following at line 1448?

return dummy->type.Rank() > 0 || dummy->ignoreTKR.test(common::IgnoreTKR::Rank);

return true;

const auto &shapeAttrs = dummy->type.attrs();
using ShapeAttrs = Fortran::evaluate::characteristics::TypeAndShape::Attr;
if (shapeAttrs.test(ShapeAttrs::AssumedRank) ||
Expand Down
12 changes: 11 additions & 1 deletion flang/lib/Lower/ConvertCall.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1255,11 +1255,21 @@ static PreparedDummyArgument preparePresentUserCallActualArgument(
passingPolymorphicToNonPolymorphic &&
(actual.isArray() || mlir::isa<fir::BaseBoxType>(dummyType));

// Helper function to make it easier to unwrap and use expression
auto argHasTriplet =
[](const Fortran::evaluate::ActualArgument &arg) -> bool {
if (const auto *expr = arg.UnwrapExpr())
return HasTriplet(*expr);
return false;
};

const bool actualHasTriplet = argHasTriplet(*arg.entity);

// The simple contiguity of the actual is "lost" when passing a polymorphic
// to a non polymorphic entity because the dummy dynamic type matters for
// the contiguity.
const bool mustDoCopyInOut =
actual.isArray() && arg.mustBeMadeContiguous() &&
actual.isArray() && arg.mustBeMadeContiguous(actualHasTriplet) &&
(passingPolymorphicToNonPolymorphic ||
!isSimplyContiguous(*arg.entity, foldingContext));

Expand Down
58 changes: 58 additions & 0 deletions flang/test/Lower/force-temp.f90
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test fail withtout your change? It seems that mustBeMadeContiguous should already return true for this case. I am just trying to understand whether I missed something in the compiler code or that you added this test for completeness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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