[flang-commits] [flang] 8ba9ee4 - [flang] Correct the subscripts used for arguments to character intrinsics
peter klausler via flang-commits
flang-commits at lists.llvm.org
Wed Jun 16 10:26:36 PDT 2021
Author: peter klausler
Date: 2021-06-16T10:26:25-07:00
New Revision: 8ba9ee46e465a56a54f8361703d3af7f4bc98d63
URL: https://github.com/llvm/llvm-project/commit/8ba9ee46e465a56a54f8361703d3af7f4bc98d63
DIFF: https://github.com/llvm/llvm-project/commit/8ba9ee46e465a56a54f8361703d3af7f4bc98d63.diff
LOG: [flang] Correct the subscripts used for arguments to character intrinsics
When chasing down another unrelated bug, I noticed that the
implementations of various character intrinsic functions assume
that the lower bounds of (some of) their arguments were 1.
This isn't necessarily the case, so I've cleaned them up, tweaked
the unit tests to exercise the fix, and regularized the allocation
pattern used for results to use SetBounds() before Allocate() rather
than the old original Descriptor::Allocate() wrapper around
CFI_allocate().
Since there were few other remaining uses of the old original
Descriptor::Allocate() wrapper, I also converted them to the
new one and deleted the old one.
Differential Revision: https://reviews.llvm.org/D104325
Added:
Modified:
flang/runtime/character.cpp
flang/runtime/descriptor.cpp
flang/runtime/descriptor.h
flang/runtime/transformational.cpp
flang/unittests/Evaluate/reshape.cpp
flang/unittests/RuntimeGTest/CharacterTest.cpp
Removed:
################################################################################
diff --git a/flang/runtime/character.cpp b/flang/runtime/character.cpp
index 826cb478770b..b927febd812c 100644
--- a/flang/runtime/character.cpp
+++ b/flang/runtime/character.cpp
@@ -83,10 +83,9 @@ static void Compare(Descriptor &result, const Descriptor &x,
RUNTIME_CHECK(
terminator, x.rank() == y.rank() || x.rank() == 0 || y.rank() == 0);
int rank{std::max(x.rank(), y.rank())};
- SubscriptValue lb[maxRank], ub[maxRank], xAt[maxRank], yAt[maxRank];
+ SubscriptValue ub[maxRank], xAt[maxRank], yAt[maxRank];
SubscriptValue elements{1};
for (int j{0}; j < rank; ++j) {
- lb[j] = 1;
if (x.rank() > 0 && y.rank() > 0) {
SubscriptValue xUB{x.GetDimension(j).Extent()};
SubscriptValue yUB{y.GetDimension(j).Extent()};
@@ -101,11 +100,15 @@ static void Compare(Descriptor &result, const Descriptor &x,
ub[j] = (x.rank() ? x : y).GetDimension(j).Extent();
}
elements *= ub[j];
- xAt[j] = yAt[j] = 1;
}
+ x.GetLowerBounds(xAt);
+ y.GetLowerBounds(yAt);
result.Establish(
TypeCategory::Logical, 1, nullptr, rank, ub, CFI_attribute_allocatable);
- if (result.Allocate(lb, ub) != CFI_SUCCESS) {
+ for (int j{0}; j < rank; ++j) {
+ result.GetDimension(j).SetBounds(1, ub[j]);
+ }
+ if (result.Allocate() != CFI_SUCCESS) {
terminator.Crash("Compare: could not allocate storage for result");
}
std::size_t xChars{x.ElementBytes() >> shift<CHAR>};
@@ -146,18 +149,21 @@ template <typename CHAR, bool ADJUSTR>
static void AdjustLRHelper(Descriptor &result, const Descriptor &string,
const Terminator &terminator) {
int rank{string.rank()};
- SubscriptValue lb[maxRank], ub[maxRank], stringAt[maxRank];
+ SubscriptValue ub[maxRank], stringAt[maxRank];
SubscriptValue elements{1};
for (int j{0}; j < rank; ++j) {
- lb[j] = 1;
ub[j] = string.GetDimension(j).Extent();
elements *= ub[j];
stringAt[j] = 1;
}
+ string.GetLowerBounds(stringAt);
std::size_t elementBytes{string.ElementBytes()};
result.Establish(string.type(), elementBytes, nullptr, rank, ub,
CFI_attribute_allocatable);
- if (result.Allocate(lb, ub) != CFI_SUCCESS) {
+ for (int j{0}; j < rank; ++j) {
+ result.GetDimension(j).SetBounds(1, ub[j]);
+ }
+ if (result.Allocate() != CFI_SUCCESS) {
terminator.Crash("ADJUSTL/R: could not allocate storage for result");
}
for (SubscriptValue resultAt{0}; elements-- > 0;
@@ -199,17 +205,19 @@ template <typename INT, typename CHAR>
static void LenTrim(Descriptor &result, const Descriptor &string,
const Terminator &terminator) {
int rank{string.rank()};
- SubscriptValue lb[maxRank], ub[maxRank], stringAt[maxRank];
+ SubscriptValue ub[maxRank], stringAt[maxRank];
SubscriptValue elements{1};
for (int j{0}; j < rank; ++j) {
- lb[j] = 1;
ub[j] = string.GetDimension(j).Extent();
elements *= ub[j];
- stringAt[j] = 1;
}
+ string.GetLowerBounds(stringAt);
result.Establish(TypeCategory::Integer, sizeof(INT), nullptr, rank, ub,
CFI_attribute_allocatable);
- if (result.Allocate(lb, ub) != CFI_SUCCESS) {
+ for (int j{0}; j < rank; ++j) {
+ result.GetDimension(j).SetBounds(1, ub[j]);
+ }
+ if (result.Allocate() != CFI_SUCCESS) {
terminator.Crash("LEN_TRIM: could not allocate storage for result");
}
std::size_t stringElementChars{string.ElementBytes() >> shift<CHAR>};
@@ -370,21 +378,27 @@ static void GeneralCharFunc(Descriptor &result, const Descriptor &string,
: arg.rank() ? arg.rank()
: back ? back->rank()
: 0};
- SubscriptValue lb[maxRank], ub[maxRank], stringAt[maxRank], argAt[maxRank],
+ SubscriptValue ub[maxRank], stringAt[maxRank], argAt[maxRank],
backAt[maxRank];
SubscriptValue elements{1};
for (int j{0}; j < rank; ++j) {
- lb[j] = 1;
ub[j] = string.rank() ? string.GetDimension(j).Extent()
: arg.rank() ? arg.GetDimension(j).Extent()
: back ? back->GetDimension(j).Extent()
: 1;
elements *= ub[j];
- stringAt[j] = argAt[j] = backAt[j] = 1;
+ }
+ string.GetLowerBounds(stringAt);
+ arg.GetLowerBounds(argAt);
+ if (back) {
+ back->GetLowerBounds(backAt);
}
result.Establish(TypeCategory::Integer, sizeof(INT), nullptr, rank, ub,
CFI_attribute_allocatable);
- if (result.Allocate(lb, ub) != CFI_SUCCESS) {
+ for (int j{0}; j < rank; ++j) {
+ result.GetDimension(j).SetBounds(1, ub[j]);
+ }
+ if (result.Allocate() != CFI_SUCCESS) {
terminator.Crash("SCAN/VERIFY: could not allocate storage for result");
}
std::size_t stringElementChars{string.ElementBytes() >> shift<CHAR>};
@@ -471,7 +485,7 @@ static void MaxMinHelper(Descriptor &accumulator, const Descriptor &x,
RUNTIME_CHECK(terminator,
accumulator.rank() == 0 || x.rank() == 0 ||
accumulator.rank() == x.rank());
- SubscriptValue lb[maxRank], ub[maxRank], xAt[maxRank];
+ SubscriptValue ub[maxRank], xAt[maxRank];
SubscriptValue elements{1};
std::size_t accumChars{accumulator.ElementBytes() >> shift<CHAR>};
std::size_t xChars{x.ElementBytes() >> shift<CHAR>};
@@ -480,10 +494,8 @@ static void MaxMinHelper(Descriptor &accumulator, const Descriptor &x,
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()};
if (accumExt != ub[j]) {
@@ -495,17 +507,20 @@ static void MaxMinHelper(Descriptor &accumulator, const Descriptor &x,
}
} else {
ub[j] = accumulator.GetDimension(j).Extent();
- xAt[j] = 1;
}
elements *= ub[j];
}
+ x.GetLowerBounds(xAt);
void *old{nullptr};
const CHAR *accumData{accumulator.OffsetElement<CHAR>()};
if (reallocate) {
old = accumulator.raw().base_addr;
accumulator.set_base_addr(nullptr);
accumulator.raw().elem_len = chars << shift<CHAR>;
- RUNTIME_CHECK(terminator, accumulator.Allocate(lb, ub) == CFI_SUCCESS);
+ for (int j{0}; j < rank; ++j) {
+ accumulator.GetDimension(j).SetBounds(1, ub[j]);
+ }
+ RUNTIME_CHECK(terminator, accumulator.Allocate() == CFI_SUCCESS);
}
for (CHAR *result{accumulator.OffsetElement<CHAR>()}; elements-- > 0;
accumData += accumChars, result += chars, x.IncrementSubscripts(xAt)) {
@@ -553,10 +568,9 @@ void RTNAME(CharacterConcatenate)(Descriptor &accumulator,
accumulator.rank() == 0 || from.rank() == 0 ||
accumulator.rank() == from.rank());
int rank{std::max(accumulator.rank(), from.rank())};
- SubscriptValue lb[maxRank], ub[maxRank], fromAt[maxRank];
+ SubscriptValue ub[maxRank], fromAt[maxRank];
SubscriptValue elements{1};
for (int j{0}; j < rank; ++j) {
- lb[j] = 1;
if (accumulator.rank() > 0 && from.rank() > 0) {
ub[j] = accumulator.GetDimension(j).Extent();
SubscriptValue fromUB{from.GetDimension(j).Extent()};
@@ -571,7 +585,6 @@ void RTNAME(CharacterConcatenate)(Descriptor &accumulator,
(accumulator.rank() ? accumulator : from).GetDimension(j).Extent();
}
elements *= ub[j];
- fromAt[j] = 1;
}
std::size_t oldBytes{accumulator.ElementBytes()};
void *old{accumulator.raw().base_addr};
@@ -579,12 +592,16 @@ void RTNAME(CharacterConcatenate)(Descriptor &accumulator,
std::size_t fromBytes{from.ElementBytes()};
accumulator.raw().elem_len += fromBytes;
std::size_t newBytes{accumulator.ElementBytes()};
- if (accumulator.Allocate(lb, ub) != CFI_SUCCESS) {
+ for (int j{0}; j < rank; ++j) {
+ accumulator.GetDimension(j).SetBounds(1, ub[j]);
+ }
+ if (accumulator.Allocate() != CFI_SUCCESS) {
terminator.Crash(
"CharacterConcatenate: could not allocate storage for result");
}
const char *p{static_cast<const char *>(old)};
char *to{static_cast<char *>(accumulator.raw().base_addr)};
+ from.GetLowerBounds(fromAt);
for (; elements-- > 0;
to += newBytes, p += oldBytes, from.IncrementSubscripts(fromAt)) {
std::memcpy(to, p, oldBytes);
@@ -601,8 +618,7 @@ void RTNAME(CharacterConcatenateScalar1)(
accumulator.set_base_addr(nullptr);
std::size_t oldLen{accumulator.ElementBytes()};
accumulator.raw().elem_len += chars;
- RUNTIME_CHECK(
- terminator, accumulator.Allocate(nullptr, nullptr) == CFI_SUCCESS);
+ RUNTIME_CHECK(terminator, accumulator.Allocate() == CFI_SUCCESS);
std::memcpy(accumulator.OffsetElement<char>(oldLen), from, chars);
FreeMemory(old);
}
@@ -650,9 +666,10 @@ void RTNAME(CharacterAssign)(Descriptor &lhs, const Descriptor &rhs,
for (int j{0}; j < rank; ++j) {
lhsAt[j] = rhsAt[j];
ub[j] = rhs.GetDimension(j).UpperBound();
+ lhs.GetDimension(j).SetBounds(lhsAt[j], ub[j]);
}
}
- RUNTIME_CHECK(terminator, lhs.Allocate(lhsAt, ub) == CFI_SUCCESS);
+ RUNTIME_CHECK(terminator, lhs.Allocate() == CFI_SUCCESS);
}
switch (lhs.raw().type) {
case CFI_type_char:
@@ -941,7 +958,7 @@ void RTNAME(Repeat)(Descriptor &result, const Descriptor &string,
std::size_t origBytes{string.ElementBytes()};
result.Establish(string.type(), origBytes * ncopies, nullptr, 0, nullptr,
CFI_attribute_allocatable);
- if (result.Allocate(nullptr, nullptr) != CFI_SUCCESS) {
+ if (result.Allocate() != CFI_SUCCESS) {
terminator.Crash("REPEAT could not allocate storage for result");
}
const char *from{string.OffsetElement()};
@@ -975,7 +992,7 @@ void RTNAME(Trim)(Descriptor &result, const Descriptor &string,
}
result.Establish(string.type(), resultBytes, nullptr, 0, nullptr,
CFI_attribute_allocatable);
- RUNTIME_CHECK(terminator, result.Allocate(nullptr, nullptr) == CFI_SUCCESS);
+ RUNTIME_CHECK(terminator, result.Allocate() == CFI_SUCCESS);
std::memcpy(result.OffsetElement(), string.OffsetElement(), resultBytes);
}
diff --git a/flang/runtime/descriptor.cpp b/flang/runtime/descriptor.cpp
index 6715afa8aee4..6747b38908ad 100644
--- a/flang/runtime/descriptor.cpp
+++ b/flang/runtime/descriptor.cpp
@@ -148,14 +148,6 @@ int Descriptor::Allocate() {
return 0;
}
-int Descriptor::Allocate(const SubscriptValue lb[], const SubscriptValue ub[]) {
- int result{ISO::CFI_allocate(&raw_, lb, ub, ElementBytes())};
- if (result == CFI_SUCCESS) {
- // TODO: derived type initialization
- }
- return result;
-}
-
int Descriptor::Deallocate(bool finalize) {
Destroy(finalize);
return ISO::CFI_deallocate(&raw_);
diff --git a/flang/runtime/descriptor.h b/flang/runtime/descriptor.h
index 5e03ad05b253..d31023b338af 100644
--- a/flang/runtime/descriptor.h
+++ b/flang/runtime/descriptor.h
@@ -304,9 +304,12 @@ class Descriptor {
std::size_t Elements() const;
+ // Allocate() assumes Elements() and ElementBytes() work;
+ // define the extents of the dimensions and the element length
+ // before calling. It (re)computes the byte strides after
+ // allocation.
// TODO: SOURCE= and MOLD=
int Allocate();
- int Allocate(const SubscriptValue lb[], const SubscriptValue ub[]);
int Deallocate(bool finalize = true);
void Destroy(bool finalize = true) const;
diff --git a/flang/runtime/transformational.cpp b/flang/runtime/transformational.cpp
index a8759595dca8..bf3618b1cf0e 100644
--- a/flang/runtime/transformational.cpp
+++ b/flang/runtime/transformational.cpp
@@ -364,13 +364,11 @@ OwningPtr<Descriptor> RTNAME(Reshape)(const Descriptor &source,
resultRank >= 0 && resultRank <= static_cast<SubscriptValue>(maxRank));
// Extract and check the shape of the result; compute its element count.
- SubscriptValue lowerBound[maxRank]; // all 1's
SubscriptValue resultExtent[maxRank];
std::size_t shapeElementBytes{shape.ElementBytes()};
std::size_t resultElements{1};
SubscriptValue shapeSubscript{shape.GetDimension(0).LowerBound()};
for (SubscriptValue j{0}; j < resultRank; ++j, ++shapeSubscript) {
- lowerBound[j] = 1;
resultExtent[j] = GetInt64(
shape.Element<char>(&shapeSubscript), shapeElementBytes, terminator);
RUNTIME_CHECK(terminator, resultExtent[j] >= 0);
@@ -434,7 +432,10 @@ OwningPtr<Descriptor> RTNAME(Reshape)(const Descriptor &source,
}
}
// Allocate storage for the result's data.
- int status{result->Allocate(lowerBound, resultExtent)};
+ 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);
}
diff --git a/flang/unittests/Evaluate/reshape.cpp b/flang/unittests/Evaluate/reshape.cpp
index c3aa8f4b76bc..719d220a1487 100644
--- a/flang/unittests/Evaluate/reshape.cpp
+++ b/flang/unittests/Evaluate/reshape.cpp
@@ -7,7 +7,6 @@ using namespace Fortran::common;
using namespace Fortran::runtime;
int main() {
- static const SubscriptValue ones[]{1, 1, 1};
static const SubscriptValue sourceExtent[]{2, 3, 4};
auto source{Descriptor::Create(TypeCategory::Integer, sizeof(std::int32_t),
nullptr, 3, sourceExtent, CFI_attribute_allocatable)};
@@ -16,7 +15,10 @@ int main() {
MATCH(sizeof(std::int32_t), source->ElementBytes());
TEST(source->IsAllocatable());
TEST(!source->IsPointer());
- TEST(source->Allocate(ones, sourceExtent) == CFI_SUCCESS);
+ for (int j{0}; j < 3; ++j) {
+ source->GetDimension(j).SetBounds(1, sourceExtent[j]);
+ }
+ TEST(source->Allocate() == CFI_SUCCESS);
TEST(source->IsAllocated());
MATCH(2, source->GetDimension(0).Extent());
MATCH(3, source->GetDimension(1).Extent());
diff --git a/flang/unittests/RuntimeGTest/CharacterTest.cpp b/flang/unittests/RuntimeGTest/CharacterTest.cpp
index 89109b1989cf..445062166700 100644
--- a/flang/unittests/RuntimeGTest/CharacterTest.cpp
+++ b/flang/unittests/RuntimeGTest/CharacterTest.cpp
@@ -30,10 +30,12 @@ OwningPtr<Descriptor> CreateDescriptor(const std::vector<SubscriptValue> &shape,
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) {
+ int rank{static_cast<int>(shape.size())};
+ // Use a weird lower bound of 2 to flush out subscripting bugs
+ for (int j{0}; j < rank; ++j) {
+ descriptor->GetDimension(j).SetBounds(2, shape[j] + 1);
+ }
+ if (descriptor->Allocate() != 0) {
return nullptr;
}
@@ -245,7 +247,7 @@ void RunExtremumTests(const char *which,
ASSERT_TRUE(x->IsAllocated());
ASSERT_NE(y, nullptr);
ASSERT_TRUE(y->IsAllocated());
- function(*x, *y, /* sourceFile = */ nullptr, /* sourceLine = */ 0);
+ function(*x, *y, __FILE__, __LINE__);
std::size_t length = x->ElementBytes() / sizeof(CHAR);
for (std::size_t i = 0; i < t.x.size(); ++i) {
@@ -296,8 +298,7 @@ void RunAllocationTest(const char *xRaw, const char *yRaw) {
ASSERT_TRUE(y->IsAllocated());
void *old = x->raw().base_addr;
- RTNAME(CharacterMin)
- (*x, *y, /* sourceFile = */ nullptr, /* sourceLine = */ 0);
+ RTNAME(CharacterMin)(*x, *y, __FILE__, __LINE__);
EXPECT_EQ(old, x->raw().base_addr);
}
More information about the flang-commits
mailing list