[flang] Update Reshape runtime library routine and its corresponding unit test

[mleair]

Summary

Update Reshape so it uses a result argument instead of a result return object. The result argument is consistent with other intrinsics and more in line with the
code generated by fir lowering.

Also, updated reshape.cpp unit test to use the new Reshape interface.

[mleair]

incorporate lint comments.

[mleair]

Remove unneeded check on pad.

[mleair]

fix address calculation for order argument.

[jeanPerier]

Looks good to me.

flang/runtime/transformational.cpp

397–401

I think auto k{GetInt64(shape.Element<char>(&orderSubscript), orderElementBytes, terminator)} might be more idiomatic than manually incrementing the byte offset. @klausler, is this correct ?

[PeteSteinfeld]

All builds, tests, and looks good to me. But I'm not very familiar with the front end's interface to the runtime.

[klausler]

flang/runtime/transformational.cpp

397–401

Yes. When something in the runtime can be written as an application of subscripts to an array, that's usually the right way to do it.

[klausler]

flang/runtime/transformational.cpp

355

needs extern "C" to match API in header

397

braces, please

[mleair]

In D104586#2829885, @jeanPerier wrote:

Looks good to me.

flang/runtime/transformational.cpp

355

Not sure what you mean. There is an extern "C" that we now share in this file above (before the Cshift runtime routine).

397–401

Unfotunately, I found a case where I was getting a bad value for k when I used the original code. That is why I rewrote it. I will try it this time with "orderElementBytes" as the second argument.

397–401

Good news. I got it to work, but after using orderElementBytes and order->Element instead of shape.Element. So, the following line appears to work:

auto k{GetInt64(order->Element<char>(&orderSubscript), orderElementBytes, terminator)};

I will make this change and update the review.

[mleair]

Incorporate comments from previous diff.

I also added an "order" argument to the reshape unit test to exercise order element calculation. I hope it's OK that the test reports 88 tests pass instead of 86. If that's a problem, I can remove the 2 extra tests.

[mleair]

@klausler @jeanPerier @PeteSteinfeld Do you have any more comments?

flang/runtime/transformational.h

 #define FORTRAN_RUNTIME_TRANSFORMATIONAL_H_
 
 #include "descriptor.h"
 #include "entry-names.h"
 #include "memory.h"
 
 namespace Fortran::runtime {
 
-// TODO: redo API, put under extern "C"
-OwningPtr<Descriptor> RTNAME(Reshape)(const Descriptor &source,
+extern "C" {
+
+void RTNAME(Reshape)(Descriptor &result, const Descriptor &source,
     const Descriptor &shape, const Descriptor *pad = nullptr,
     const Descriptor *order = nullptr, const char *sourceFile = nullptr,
     int line = 0);
 
-extern "C" {
-
 void RTNAME(Cshift)(Descriptor &result, const Descriptor &source,
     const Descriptor &shift, int dim = 1, const char *sourceFile = nullptr,
     int line = 0);
 void RTNAME(CshiftVector)(Descriptor &result, const Descriptor &source,
     std::int64_t shift, const char *sourceFile = nullptr, int line = 0);
 
 void RTNAME(Eoshift)(Descriptor &result, const Descriptor &source,
     const Descriptor &shift, const Descriptor *boundary = nullptr, int dim = 1,

flang/runtime/transformational.cpp

if (vector) {
     SubscriptValue vectorAt{
         vector->GetDimension(0).LowerBound() + resultAt - 1};
     for (; resultAt <= extent; ++resultAt, ++vectorAt) {
       CopyElement(result, &resultAt, *vector, &vectorAt, terminator);
     }
   }
 }
 
-} // extern "C" - TODO put Reshape under extern "C"
+// RESHAPE
 // F2018 16.9.163
-OwningPtr<Descriptor> RTNAME(Reshape)(const Descriptor &source,
+void RTNAME(Reshape)(Descriptor &result, const Descriptor &source,
     const Descriptor &shape, const Descriptor *pad, const Descriptor *order,
     const char *sourceFile, int line) {
   // Compute and check the rank of the result.
   Terminator terminator{sourceFile, line};
   RUNTIME_CHECK(terminator, shape.rank() == 1);
   RUNTIME_CHECK(terminator, shape.type().IsInteger());
   SubscriptValue resultRank{shape.GetDimension(0).Extent()};
   RUNTIME_CHECK(terminator,
void RTNAME(Reshape)(Descriptor &result, const Descriptor &source,
   // permutation of [1..resultRank].
   int dimOrder[maxRank];
   if (order) {
     RUNTIME_CHECK(terminator, order->rank() == 1);
     RUNTIME_CHECK(terminator, order->type().IsInteger());
     RUNTIME_CHECK(terminator, order->GetDimension(0).Extent() == resultRank);
     std::uint64_t values{0};
     SubscriptValue orderSubscript{order->GetDimension(0).LowerBound()};
+    std::size_t orderElementBytes{order->ElementBytes()};
     for (SubscriptValue j{0}; j < resultRank; ++j, ++orderSubscript) {
-      auto k{GetInt64(order->OffsetElement<char>(orderSubscript),
-          shapeElementBytes, terminator)};
+      auto k{GetInt64(order->Element<char>(&orderSubscript), orderElementBytes,
+          terminator)};
       RUNTIME_CHECK(
           terminator, k >= 1 && k <= resultRank && !((values >> k) & 1));
       values |= std::uint64_t{1} << k;
       dimOrder[k - 1] = j;
     }
   } else {
     for (int j{0}; j < resultRank; ++j) {
       dimOrder[j] = j;
     }
   }
 
-  // Create and populate the result's descriptor.
-  const DescriptorAddendum *sourceAddendum{source.Addendum()};
-  const typeInfo::DerivedType *sourceDerivedType{
-      sourceAddendum ? sourceAddendum->derivedType() : nullptr};
-  OwningPtr<Descriptor> result;
-  if (sourceDerivedType) {
-    result = Descriptor::Create(*sourceDerivedType, nullptr, resultRank,
-        resultExtent, CFI_attribute_allocatable);
-  } else {
-    result = Descriptor::Create(source.type(), elementBytes, nullptr,
-        resultRank, resultExtent,
-        CFI_attribute_allocatable); // TODO rearrange these arguments
-  }
-  DescriptorAddendum *resultAddendum{result->Addendum()};
-  RUNTIME_CHECK(terminator, resultAddendum);
-  resultAddendum->flags() |= DescriptorAddendum::DoNotFinalize;
-  if (sourceDerivedType) {
-    std::size_t lenParameters{sourceAddendum->LenParameters()};
-    for (std::size_t j{0}; j < lenParameters; ++j) {
-      resultAddendum->SetLenParameterValue(
-          j, sourceAddendum->LenParameterValue(j));
-    }
-  }
-  // Allocate storage for the result's data.
-  for (int j{0}; j < resultRank; ++j) {
-    result->GetDimension(j).SetBounds(1, resultExtent[j]);
-  }
-  int status{result->Allocate()};
-  if (status != CFI_SUCCESS) {
-    terminator.Crash("RESHAPE: Allocate failed (error %d)", status);
-  }
+  // Allocate result descriptor
+  AllocateResult(
+      result, source, resultRank, resultExtent, terminator, "RESHAPE");
 
   // Populate the result's elements.
   SubscriptValue resultSubscript[maxRank];
-  result->GetLowerBounds(resultSubscript);
+  result.GetLowerBounds(resultSubscript);
   SubscriptValue sourceSubscript[maxRank];
   source.GetLowerBounds(sourceSubscript);
   std::size_t resultElement{0};
   std::size_t elementsFromSource{std::min(resultElements, sourceElements)};
   for (; resultElement < elementsFromSource; ++resultElement) {
-    CopyElement(*result, resultSubscript, source, sourceSubscript, terminator);
+    CopyElement(result, resultSubscript, source, sourceSubscript, terminator);
     source.IncrementSubscripts(sourceSubscript);
-    result->IncrementSubscripts(resultSubscript, dimOrder);
+    result.IncrementSubscripts(resultSubscript, dimOrder);
   }
   if (resultElement < resultElements) {
     // Remaining elements come from the optional PAD= argument.
     SubscriptValue padSubscript[maxRank];
     pad->GetLowerBounds(padSubscript);
     for (; resultElement < resultElements; ++resultElement) {
-      CopyElement(*result, resultSubscript, *pad, padSubscript, terminator);
+      CopyElement(result, resultSubscript, *pad, padSubscript, terminator);
       pad->IncrementSubscripts(padSubscript);
-      result->IncrementSubscripts(resultSubscript, dimOrder);
+      result.IncrementSubscripts(resultSubscript, dimOrder);
     }
   }
-
-  return result;
 }
-extern "C" { // TODO - remove when Reshape is under extern "C"
 
 // SPREAD
 void RTNAME(Spread)(Descriptor &result, const Descriptor &source, int dim,
     std::int64_t ncopies, const char *sourceFile, int line) {
   Terminator terminator{sourceFile, line};
   int rank{source.rank() + 1};
   RUNTIME_CHECK(terminator, rank <= maxRank);
   ncopies = std::max<std::int64_t>(ncopies, 0);

flang/unittests/Evaluate/reshape.cpp

pad.Establish(TypeCategory::Integer, static_cast<int>(sizeof padData[0]),
       const_cast<void *>(reinterpret_cast<const void *>(padData)), 3, padExtent,
       CFI_attribute_pointer);
   padDescriptor.Check();
   pad.Check();
   MATCH(3, pad.rank());
   MATCH(2, pad.GetDimension(0).Extent());
   MATCH(2, pad.GetDimension(1).Extent());
   MATCH(3, pad.GetDimension(2).Extent());
+  StaticDescriptor<1> orderDescriptor;
+  Descriptor &order{orderDescriptor.descriptor()};
+  static const std::int32_t orderData[]{1, 2};
+  static const SubscriptValue orderExtent[]{2};
+  order.Establish(TypeCategory::Integer, static_cast<int>(sizeof orderData[0]),
+      const_cast<void *>(reinterpret_cast<const void *>(orderData)), 1,
+      orderExtent, CFI_attribute_pointer);
+  orderDescriptor.Check();
+  order.Check();
+  MATCH(1, order.rank());
+  MATCH(2, order.GetDimension(0).Extent());
 
-  auto result{
-      RTNAME(Reshape)(*source, *shape, &pad, nullptr, __FILE__, __LINE__)};
+  auto result{Descriptor::Create(TypeCategory::Integer, sizeof(std::int32_t),
+      nullptr, 2, nullptr, CFI_attribute_allocatable)};
   TEST(result.get() != nullptr);
+  RTNAME(Reshape)(*result, *source, *shape, &pad, &order, __FILE__, __LINE__);
   result->Check();
   MATCH(sizeof(std::int32_t), result->ElementBytes());
   MATCH(2, result->rank());
   TEST(result->type().IsInteger());
   for (std::int32_t j{0}; j < 32; ++j) {
     MATCH(j, *result->OffsetElement<std::int32_t>(j * sizeof(std::int32_t)));
   }
   for (std::int32_t j{0}; j < 32; ++j) {
     SubscriptValue ss[2]{1 + (j % 8), 1 + (j / 8)};
     MATCH(j, *result->Element<std::int32_t>(ss));
   }
 
-  // TODO: test ORDER=
-
   return testing::Complete();
 }