[flang-commits] [flang] 2ea36e9 - [flang] Remove redundant reallocation

[Diana Picus via flang-commits]

Author: Diana Picus
Date: 2021-05-07T08:54:09Z
New Revision: 2ea36e94927ccbc1f8e915a4e5c932531e69f02d

URL: https://github.com/llvm/llvm-project/commit/2ea36e94927ccbc1f8e915a4e5c932531e69f02d
DIFF: https://github.com/llvm/llvm-project/commit/2ea36e94927ccbc1f8e915a4e5c932531e69f02d.diff

LOG: [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

Added: 
    

Modified: 
    flang/runtime/character.cpp
    flang/unittests/RuntimeGTest/CharacterTest.cpp

Removed: 
    


################################################################################
diff  --git a/flang/runtime/character.cpp b/flang/runtime/character.cpp
index ed8e5f4984f8..e1e529d988b6 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 2088c06ea6f6..7d90ebf0a5fc 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>