[flang] Consolidate copy-in/copy-out determination in evaluate framework #151408
Conversation
Plumbing/API for copy-in/copy-out
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
It might be cleaner to do the analysis at the time that the ProcedureRef is created by expression semantics rather than patching the IR later in lowering. It could then be used in Semantics/check-call.cpp for all of the errors and warnings that depend on copy-in/copy-out instead of the ad-hoc analysis currently done that relies on contiguity &c.
interfaces. Certain helper routines gained overloads to work with ActualArgument.
flang/include/flang/Evaluate/call.h
Outdated
| @@ -110,6 +110,7 @@ class ActualArgument { | |||
|
|
|||
| std::optional<DynamicType> GetType() const; | |||
| int Rank() const; | |||
| bool IsArray() const; | |||
There was a problem hiding this comment.
Is IsArray() redundant with Rank() > 0? If not, how do they differ -- assumed-rank? (In the terminology of the Fortran standard, "assumed-rank" dummy arguments are actually not arrays.)
There was a problem hiding this comment.
In the terminology of the Fortran standard, "assumed-rank" dummy arguments are actually not arrays.
To avoid confusion, I'll move the checks elsewhere.
| @@ -248,6 +254,7 @@ struct DummyDataObject { | |||
| bool CanBePassedViaImplicitInterface(std::string *whyNot = nullptr) const; | |||
| bool IsPassedByDescriptor(bool isBindC) const; | |||
| llvm::raw_ostream &Dump(llvm::raw_ostream &) const; | |||
| bool IsArray() const; | |||
There was a problem hiding this comment.
Same as above, is this redundant with Rank() > 0?
| (dummyTreatAsArray && !dummyIsPolymorphic) || dummyIsVoidStar || | ||
| dummyObj_.attrs.test( | ||
| characteristics::DummyDataObject::Attr::Contiguous)}; | ||
| if (!actualTreatAsContiguous && dummyNeedsContiguity) { |
There was a problem hiding this comment.
Last four lines could be a single return statement.
| const characteristics::DummyDataObject &dummyObj_; | ||
| }; | ||
|
|
||
| static bool MayNeedCopyIn(FoldingContext &fc, const ActualArgument &actual, |
There was a problem hiding this comment.
Can you combine this function with the next one? It might be clearer and more maintainable.
There was a problem hiding this comment.
Ok, simplified it further
| // non-contiguous. | ||
| // Copy-out: vector subscripts could refer to duplicate elements, can't | ||
| // copy out. | ||
| return forCopyOut ? !HasVectorSubscript(*actual) : true; |
There was a problem hiding this comment.
!(forCopyOut && HasVectorSubscript(...)) would be clearer, I think.
Backport of llvm#151408 New implementation of `MayNeedCopy()` is used to consolidate copy-in/copy-out checks. `IsAssumedShape()` and `IsAssumedRank()` were simplified and are both now in `Fortran::semantics` workspace. `preparePresentUserCallActualArgument()` in lowering was modified to use `MayNeedCopyInOut()` Fixes llvm#138471 Conflicts in backport were trivial. Remove modifications of new code not present in 21.x branch: flang/lib/Semantics/check-omp-structure.cpp flang/lib/Semantics/resolve-directives.cpp
New implementation of
MayNeedCopy()is used to consolidate copy-in/copy-out checks.IsAssumedShape()andIsAssumedRank()were simplified and are both now inFortran::semanticsworkspace.preparePresentUserCallActualArgument()in lowering was modified to useMayNeedCopyInOut()Fixes #138471