[flang] Remove redundant reallocation

[rovka]

Summary

The MaxMinHelper used to implement MIN and MAX for character types would
reallocate the accumulator whenever the number of characters in it was
different from that in the other input. This is unnecessary if the
accumulator is already larger than the other input. This patch fixes the
issue and adds a unit test to make sure we don't reallocate if we don't
need to.

[klausler]

Thanks for noticing and fixing this, and for the new test!

flang/runtime/character.cpp

RUNTIME_CHECK(terminator,
       accumulator.rank() == 0 || x.rank() == 0 ||
           accumulator.rank() == x.rank());
   SubscriptValue lb[maxRank], ub[maxRank], xAt[maxRank];
   SubscriptValue elements{1};
   std::size_t accumChars{accumulator.ElementBytes() >> shift<CHAR>};
   std::size_t xChars{x.ElementBytes() >> shift<CHAR>};
   std::size_t chars{std::max(accumChars, xChars)};
   bool reallocate{accumulator.raw().base_addr == nullptr ||
-      accumChars != xChars || (accumulator.rank() == 0 && x.rank() > 0)};
+      accumChars != chars || (accumulator.rank() == 0 && x.rank() > 0)};
   int rank{std::max(accumulator.rank(), x.rank())};
   for (int j{0}; j < rank; ++j) {
     lb[j] = 1;
     if (x.rank() > 0) {
       ub[j] = x.GetDimension(j).Extent();
       xAt[j] = x.GetDimension(j).LowerBound();
       if (accumulator.rank() > 0) {
         SubscriptValue accumExt{accumulator.GetDimension(j).Extent()};

flang/unittests/RuntimeGTest/CharacterTest.cpp

 }
 
 // Test MIN() and MAX()
 struct ExtremumTestCase {
   std::vector<SubscriptValue> shape; // Empty = scalar, non-empty = array.
   std::vector<const char *> x, y, expect;
 };
 
-template <typename CHAR>
-void RunExtremumTests(const char *which,
-    std::function<void(Descriptor &, const Descriptor &, const char *, int)>
-        function,
-    const std::vector<ExtremumTestCase> &testCases) {
-
-  // Helper for creating, allocating and filling up a descriptor with data from
-  // raw character literals, converted to the CHAR type used by the test.
-  auto CreateDescriptor = [](const std::vector<SubscriptValue> &shape,
-                              const std::vector<const char *> &raw_strings)
-      -> OwningPtr<Descriptor> {
-    std::size_t length{std::strlen(raw_strings[0])};
+template <typename CHAR>
+OwningPtr<Descriptor> CreateDescriptor(const std::vector<SubscriptValue> &shape,
+    const std::vector<const char *> &raw_strings) {
 
-    OwningPtr<Descriptor> descriptor{Descriptor::Create(sizeof(CHAR), length,
-        nullptr, shape.size(), nullptr, CFI_attribute_allocatable)};
-    if ((shape.empty()
-                ? descriptor->Allocate()
+  if ((shape.empty() ? descriptor->Allocate()
                      : descriptor->Allocate(
                            std::vector<SubscriptValue>(shape.size(), 1).data(),
                            shape.data())) != 0) {
-      return nullptr;
-    }
 
-    std::size_t offset = 0;
-    for (const char *raw : raw_strings) {
-      std::basic_string<CHAR> converted{raw, raw + length};
-      std::copy(converted.begin(), converted.end(),
-          descriptor->OffsetElement<CHAR>(offset * length * sizeof(CHAR)));
-      ++offset;
-    }
 
-    return descriptor;
-  };
+}
 
+template <typename CHAR>
+void RunExtremumTests(const char *which,
+    std::function<void(Descriptor &, const Descriptor &, const char *, int)>
+        function,
+    const std::vector<ExtremumTestCase> &testCases) {
   std::stringstream traceMessage;
   traceMessage << which << " for CHARACTER(kind=" << sizeof(CHAR) << ")";
   SCOPED_TRACE(traceMessage.str());
 
   for (const auto &t : testCases) {
-    OwningPtr<Descriptor> x = CreateDescriptor(t.shape, t.x);
-    OwningPtr<Descriptor> y = CreateDescriptor(t.shape, t.y);
+    OwningPtr<Descriptor> x = CreateDescriptor<CHAR>(t.shape, t.x);
+    OwningPtr<Descriptor> y = CreateDescriptor<CHAR>(t.shape, t.y);
 
     ASSERT_NE(x, nullptr);
     ASSERT_TRUE(x->IsAllocated());
     ASSERT_NE(y, nullptr);
     ASSERT_TRUE(y->IsAllocated());
     function(*x, *y, /* sourceFile = */ nullptr, /* sourceLine = */ 0);
 
     std::size_t length = x->ElementBytes() / sizeof(CHAR);
TYPED_TEST(ExtremumTests, MaxTests) {
   static std::vector<ExtremumTestCase> tests{
       {{}, {"a"}, {"z"}, {"z"}},
       {{1}, {"zaa"}, {"aaaaa"}, {"zaa  "}},
       {{1, 1, 1}, {"aaaaa"}, {"aazaa"}, {"aazaa"}},
   };
   RunExtremumTests<TypeParam>("MAX", RTNAME(CharacterMax), tests);
 }
 
+template <typename CHAR>
+void RunAllocationTest(const char *xRaw, const char *yRaw) {
+  OwningPtr<Descriptor> x = CreateDescriptor<CHAR>({}, {xRaw});
+  OwningPtr<Descriptor> y = CreateDescriptor<CHAR>({}, {yRaw});
+
+  ASSERT_NE(x, nullptr);
+  ASSERT_TRUE(x->IsAllocated());
+  ASSERT_NE(y, nullptr);
+  ASSERT_TRUE(y->IsAllocated());
+
+  void *old = x->raw().base_addr;
+  RTNAME(CharacterMin)
+  (*x, *y, /* sourceFile = */ nullptr, /* sourceLine = */ 0);
+  EXPECT_EQ(old, x->raw().base_addr);
+}
+
+TYPED_TEST(ExtremumTests, NoReallocate) {
+  // Test that we don't reallocate if the accumulator is already large enough.
+  RunAllocationTest<TypeParam>("loooooong", "short");
+}
+
 // Test search functions INDEX(), SCAN(), and VERIFY()
 
 template <typename CHAR>
 using SearchFunction = std::function<std::size_t(
     const CHAR *, std::size_t, const CHAR *, std::size_t, bool)>;
 template <template <typename> class FUNC>
 using CharTypedFunctions =
     std::tuple<FUNC<char>, FUNC<char16_t>, FUNC<char32_t>>;