[flang-commits] [flang] [flang] Fixed regression in copy-in/copy-out (PR #161259)

[via flang-commits]

================
@@ -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)) {
----------------
jeanPerier wrote:

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<bool>, where  `std::nullopt` means not sure, or just revert lowering to use its owns 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`).

https://github.com/llvm/llvm-project/pull/161259