From 2ea36e94927ccbc1f8e915a4e5c932531e69f02d Mon Sep 17 00:00:00 2001
From: Diana Picus <diana.picus@linaro.org>
Date: Thu, 6 May 2021 09:26:57 +0000
Subject: [PATCH] [flang] Remove redundant reallocation

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.

Differential Revision: https://reviews.llvm.org/D101984
---
 flang/runtime/character.cpp                    |  2 +-
 flang/unittests/RuntimeGTest/CharacterTest.cpp | 81 ++++++++++++++++----------
 2 files changed, 51 insertions(+), 32 deletions(-)

diff --git a/flang/runtime/character.cpp b/flang/runtime/character.cpp
index ed8e5f4..e1e529d 100644
--- a/flang/runtime/character.cpp
+++ b/flang/runtime/character.cpp
@@ -477,7 +477,7 @@ static void MaxMinHelper(Descriptor &accumulator, const Descriptor &x,
   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;
diff --git a/flang/unittests/RuntimeGTest/CharacterTest.cpp b/flang/unittests/RuntimeGTest/CharacterTest.cpp
index 2088c06..7d90ebf 100644
--- a/flang/unittests/RuntimeGTest/CharacterTest.cpp
+++ b/flang/unittests/RuntimeGTest/CharacterTest.cpp
@@ -146,47 +146,45 @@ struct ExtremumTestCase {
   std::vector<const char *> x, y, expect;
 };
 
+// Helper for creating, allocating and filling up a descriptor with data from
+// raw character literals, converted to the CHAR type used by the test.
+template <typename CHAR>
+OwningPtr<Descriptor> CreateDescriptor(const std::vector<SubscriptValue> &shape,
+    const std::vector<const char *> &raw_strings) {
+  std::size_t length{std::strlen(raw_strings[0])};
+
+  OwningPtr<Descriptor> descriptor{Descriptor::Create(sizeof(CHAR), length,
+      nullptr, shape.size(), nullptr, CFI_attribute_allocatable)};
+  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) {
-
-  // 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])};
-
-    OwningPtr<Descriptor> descriptor{Descriptor::Create(sizeof(CHAR), length,
-        nullptr, shape.size(), nullptr, CFI_attribute_allocatable)};
-    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;
-  };
-
   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());
@@ -232,6 +230,27 @@ TYPED_TEST(ExtremumTests, MaxTests) {
   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>
-- 
2.7.4