Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flang] Re-land PR#97337 #98656

Merged
merged 2 commits into from
Jul 12, 2024
Merged

[flang] Re-land PR#97337 #98656

merged 2 commits into from
Jul 12, 2024

Conversation

klausler
Copy link
Contributor

Pull request #97337 was
reverted by #98612 due
to two failing tests in llvm-test-suite -- which I ran, as always,
but must have bungled or misinterpreted (mea culpa).

The failing tests were llvm-test-suite/Fortran/gfortran/regression/
char_length_{20,21}.f90. They have array constructors with
explicit character types whose dynamic length values are negative
at runtime, which must be interpreted as zero.

This patch extends the original to cover those cases.

An implied DO loop with no trips in an array constructor does not have
a well-defined character length unless its data items have a length that
is constant expression.  That works, but the implementation is too broadly
applied.  An array constructor with an explicit type-spec always has a
well-defined length.
@klausler klausler requested a review from psteinfeld July 12, 2024 16:14
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:semantics labels Jul 12, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 12, 2024

@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-flang-semantics

Author: Peter Klausler (klausler)

Changes

Pull request #97337 was
reverted by #98612 due
to two failing tests in llvm-test-suite -- which I ran, as always,
but must have bungled or misinterpreted (mea culpa).

The failing tests were llvm-test-suite/Fortran/gfortran/regression/
char_length_{20,21}.f90. They have array constructors with
explicit character types whose dynamic length values are negative
at runtime, which must be interpreted as zero.

This patch extends the original to cover those cases.


Full diff: https://github.com/llvm/llvm-project/pull/98656.diff

3 Files Affected:

  • (modified) flang/lib/Semantics/expression.cpp (+12-4)
  • (modified) flang/test/Lower/HLFIR/array-ctor-character.f90 (+5-4)
  • (modified) flang/test/Semantics/array-constr-len.f90 (-1)
diff --git a/flang/lib/Semantics/expression.cpp b/flang/lib/Semantics/expression.cpp
index 90900d9acb6ad..2d91962a24a4c 100644
--- a/flang/lib/Semantics/expression.cpp
+++ b/flang/lib/Semantics/expression.cpp
@@ -63,7 +63,7 @@ std::optional<Expr<SubscriptInteger>> DynamicTypeWithLength::LEN() const {
 }
 
 static std::optional<DynamicTypeWithLength> AnalyzeTypeSpec(
-    const std::optional<parser::TypeSpec> &spec) {
+    const std::optional<parser::TypeSpec> &spec, FoldingContext &context) {
   if (spec) {
     if (const semantics::DeclTypeSpec *typeSpec{spec->declTypeSpec}) {
       // Name resolution sets TypeSpec::declTypeSpec only when it's valid
@@ -80,7 +80,13 @@ static std::optional<DynamicTypeWithLength> AnalyzeTypeSpec(
             const semantics::ParamValue &len{cts.length()};
             // N.B. CHARACTER(LEN=*) is allowed in type-specs in ALLOCATE() &
             // type guards, but not in array constructors.
-            return DynamicTypeWithLength{DynamicType{kind, len}};
+            DynamicTypeWithLength type{DynamicType{kind, len}};
+            if (auto lenExpr{type.LEN()}) {
+              type.length = Fold(context,
+                  AsExpr(Extremum<SubscriptInteger>{Ordering::Greater,
+                      Expr<SubscriptInteger>{0}, std::move(*lenExpr)}));
+            }
+            return type;
           } else {
             return DynamicTypeWithLength{DynamicType{category, kind}};
           }
@@ -1584,7 +1590,8 @@ class ArrayConstructorContext {
   std::optional<Expr<SubscriptInteger>> LengthIfGood() const {
     if (type_) {
       auto len{type_->LEN()};
-      if (len && IsConstantExpr(*len) && !ContainsAnyImpliedDoIndex(*len)) {
+      if (explicitType_ ||
+          (len && IsConstantExpr(*len) && !ContainsAnyImpliedDoIndex(*len))) {
         return len;
       }
     }
@@ -1939,7 +1946,8 @@ MaybeExpr ArrayConstructorContext::ToExpr() {
 
 MaybeExpr ExpressionAnalyzer::Analyze(const parser::ArrayConstructor &array) {
   const parser::AcSpec &acSpec{array.v};
-  ArrayConstructorContext acContext{*this, AnalyzeTypeSpec(acSpec.type)};
+  ArrayConstructorContext acContext{
+      *this, AnalyzeTypeSpec(acSpec.type, GetFoldingContext())};
   for (const parser::AcValue &value : acSpec.values) {
     acContext.Add(value);
   }
diff --git a/flang/test/Lower/HLFIR/array-ctor-character.f90 b/flang/test/Lower/HLFIR/array-ctor-character.f90
index 85e6ed27fe077..881085b370ffe 100644
--- a/flang/test/Lower/HLFIR/array-ctor-character.f90
+++ b/flang/test/Lower/HLFIR/array-ctor-character.f90
@@ -93,10 +93,11 @@ subroutine test_set_length_sanitize(i, c1)
   call takes_char([character(len=i):: c1])
 end subroutine
 ! CHECK-LABEL:   func.func @_QPtest_set_length_sanitize(
-! CHECK:   %[[VAL_6:.*]]:2 = hlfir.declare {{.*}}Ec1
-! CHECK:   %[[VAL_9:.*]]:2 = hlfir.declare {{.*}}Ei
-! CHECK:   %[[VAL_25:.*]] = fir.load %[[VAL_9]]#0 : !fir.ref<i64>
+! CHECK:   %[[VAL_2:.*]]:2 = hlfir.declare {{.*}}Ec1
+! CHECK:   %[[VAL_3:.*]]:2 = hlfir.declare %arg0
+! CHECK:   %[[VAL_4:.*]] = fir.load %[[VAL_3]]#0 : !fir.ref<i64>
+! CHECK:   %[[VAL_25:.*]] = fir.load %[[VAL_3]]#0 : !fir.ref<i64>
 ! CHECK:   %[[VAL_26:.*]] = arith.constant 0 : i64
 ! CHECK:   %[[VAL_27:.*]] = arith.cmpi sgt, %[[VAL_25]], %[[VAL_26]] : i64
 ! CHECK:   %[[VAL_28:.*]] = arith.select %[[VAL_27]], %[[VAL_25]], %[[VAL_26]] : i64
-! CHECK:   %[[VAL_29:.*]] = hlfir.set_length %[[VAL_6]]#0 len %[[VAL_28]] : (!fir.boxchar<1>, i64) -> !hlfir.expr<!fir.char<1,?>>
+! CHECK:   %[[VAL_29:.*]] = hlfir.set_length %[[VAL_2]]#0 len %[[VAL_28]] : (!fir.boxchar<1>, i64) -> !hlfir.expr<!fir.char<1,?>>
diff --git a/flang/test/Semantics/array-constr-len.f90 b/flang/test/Semantics/array-constr-len.f90
index a785c9e2ece6d..4de9c76c7041c 100644
--- a/flang/test/Semantics/array-constr-len.f90
+++ b/flang/test/Semantics/array-constr-len.f90
@@ -10,6 +10,5 @@ subroutine subr(s,n)
   print *, [(s(1:j),j=1,0)]
   print *, [(s(1:1),j=1,0)] ! ok
   print *, [character(2)::(s(1:n),j=1,0)] ! ok
-  !ERROR: Array constructor implied DO loop has no iterations and indeterminate character length
   print *, [character(n)::(s(1:n),j=1,0)]
 end

Pull request llvm#97337 was
reverted by llvm#98612 due
to two failing tests in llvm-test-suite -- which I ran, as always,
but must have bungled or misinterpreted (mea culpa).

The failing tests were llvm-test-suite/Fortran/gfortran/regression/
char_length_{20,21}.f90.  They have array constructors with
explicit character types whose dynamic length values are negative
at runtime and which must be interpreted as zero.

This patch extends the original to cover those cases.
Copy link
Contributor

@clementval clementval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@klausler klausler merged commit 7f06560 into llvm:main Jul 12, 2024
5 of 6 checks passed
@klausler klausler deleted the unrevert branch July 12, 2024 19:29
aaryanshukla pushed a commit to aaryanshukla/llvm-project that referenced this pull request Jul 14, 2024
Pull request llvm#97337 was
reverted by llvm#98612 due
to two failing tests in llvm-test-suite -- which I ran, as always,
but must have bungled or misinterpreted (mea culpa).
    
The failing tests were llvm-test-suite/Fortran/gfortran/regression/
char_length_{20,21}.f90.  They have array constructors with
explicit character types whose dynamic length values are negative
at runtime, which must be interpreted as zero.
    
This patch extends the original to cover those cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang:semantics flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants