[flang] Fix UBOUND() folding for constant arrays

[FruitClover]

Summary

Similarly to LBOUND in https://reviews.llvm.org/D123237, fix UBOUND() folding
for constant arrays (for both w/ and w/o DIM=): convert
GetConstantArrayLboundHelper into common helper class for both lower/upper
bounds.

[FruitClover]

flang/lib/Evaluate/fold-integer.cpp

77–78

Don't know which type is preferred in FE - std::enable_if or maybe constexpr

[FruitClover]

upd. code comment

[clementval]

Are you missing a dependent patch? Patch application is failing

[FruitClover]

In D123520#3443119, @clementval wrote:

Are you missing a dependent patch? Patch application is failing

Hello, just wanted to push a patch based on https://reviews.llvm.org/D123237#3443084 (which is still in review) to mention similar problem. (Can I somehow set base to different review HEAD, not main branch?)

[rovka]

In D123520#3443131, @FruitClover wrote:

In D123520#3443119, @clementval wrote:

Are you missing a dependent patch? Patch application is failing

Hello, just wanted to push a patch based on https://reviews.llvm.org/D123237#3443084 (which is still in review) to mention similar problem. (Can I somehow set base to different review HEAD, not main branch?)

If you're using the web interface, you can click 'Edit related revisions', then 'Edit parent revisions'. If you're using arc, I think you can add "Depends on D123237" to the commit message. See docs.

[jeanPerier]

flang/lib/Evaluate/fold-integer.cpp

34

I think the template makes this helper class interface a bit too complex for what it does. I am not sure it is worth making this a template parameter (templates are nice, but they have f18 compile time cost and f18 executable size costs, in this case, I do not think the template adds a lot of type safety).

What about doing having an interface like:

static Expr<T> GetConstantArrayBound::GetLbound(const Expr<SomeType>&, std::optional<int> dim);
and
static Expr<T> GetConstantArrayBound::GetUbound(const Expr<SomeType>&, std::optional<int> dim);

with the implementation looking something like:

template <typename T>
Expr<T> PackageConstantBounds(
    const ConstantSubscripts &&bounds, bool asScalar = false) {
  // .... like  PackageConstant, but returns Expr<T>
}

class GetConstantArrayBound {
public:
   static Expr<T> GetConstantArrayBound::GetLbound(const Expr<SomeType>& array, std::optional<ConstantSubscript> dim) {
     return PackageConstantBounds<T>(GetConstantArrayBound(dim, /*getLbound=*/true)::Get(array), dim.has_value());
  }
  //... same for GetUbound
private:
    // Private ctor ...

    ConstantSubscripts Get(const Constant<T> &x) {
      if (getLbound) { 
        // Return the lower bound
        if (dim_) {
          return {x.lbounds().at(*dim_)};
        } else {
          return x.lbounds();
        }
      } else {
        return  x.ComputeUbounds(dim_);
      }
  }

  // Deal with the const Parentheses<T> &x. (return ones for lbouds, and the left operand shape for ubound).
  
  const std::optional<ConstantSubscript> dim_;
  const bool getLBound_;
}
  
}

[FruitClover]

Update after review

[FruitClover]

upd. base

[FruitClover]

flang/lib/Evaluate/fold-integer.cpp

34

Thanks a lot for detailed feedback, suggested change looks much better, updated.

[FruitClover]

flang/lib/Evaluate/fold-integer.cpp

77–81

Proper UBOUD((x)) is done in https://reviews.llvm.org/D123944 fyi (Did not want to mix changes and simplify review a bit)

[FruitClover]

clang-format

[jeanPerier]

Other than my inlined comment about UBOUND of (cst), this looks good.

flang/lib/Evaluate/fold-integer.cpp

77

The UBOUND case if missing for the Parentheses<T> case (need to return the shape). You can probably use GetConstantExtents if you make the FoldingContext& available here, or you can set a flag that parentheses were met and return x.shape() in Get(const Constant<T> &x) in that case.

[FruitClover]

flang/lib/Evaluate/fold-integer.cpp

77

It's very next PR in sequence (Revision Contents -> Stack) - https://reviews.llvm.org/D123944, could combine into 1 patch if that's better.

[FruitClover]

Rebase

[jeanPerier]

Looks good, thanks for bearing with the comments !

[FruitClover]

In D123520#3466988, @jeanPerier wrote:

Looks good, thanks for bearing with the comments !

Thanks a lot for review, @jeanPerier !

[FruitClover]

Windows buildbot failed https://lab.llvm.org/buildbot/#/builders/172/builds/11593 after this patch in flang\test\Semantics\select-rank.f90:

******************** TEST 'Flang :: Semantics/select-rank.f90' FAILED ********************
Script:
--
: 'RUN: at line 1';   "C:\Python310\python.exe" C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\flang\test\Semantics/test_errors.py C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\flang\test\Semantics\select-rank.f90 c:\users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\build\bin\flang-new.exe -fc1
--
Exit Code: 1
Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "C:\Python310\python.exe" "C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\flang\test\Semantics/test_errors.py" "C:\Users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\llvm-project\flang\test\Semantics\select-rank.f90" "c:\users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\build\bin\flang-new.exe" "-fc1"
# command output:
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.	Program arguments: c:\\users\\buildbot-worker\\minipc-ryzen-win\\flang-x86_64-windows\\build\\bin\\flang-new.exe -fc1 -fsyntax-only C:\\Users\\buildbot-worker\\minipc-ryzen-win\\flang-x86_64-windows\\llvm-project\\flang\\test\\Semantics\\select-rank.f90
#0 0x00007ff786545e9d (c:\users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\build\bin\flang-new.exe+0xfc5e9d)
#1 0x00007ff786561245 (c:\users\buildbot-worker\minipc-ryzen-win\flang-x86_64-windows\build\bin\flang-new.exe+0xfe1245)
#2 0x0000000000000068 
#3 0x0000000000000007 
error: command failed with exit status: 1
--
********************

but for now I don't understand the root cause yet (no lbound/ubound in testcase)
(CC @jeanPerier / @klausler )

[jeanPerier]

In D123520#3477672, @FruitClover wrote:

Windows buildbot failed https://lab.llvm.org/buildbot/#/builders/172/builds/11593 after this patch in flang\test\Semantics\select-rank.f90:
[...]
but for now I don't understand the root cause yet (no lbound/ubound in testcase)
(CC @jeanPerier / @klausler )

I do not understand the failure either. It seems the windows bot is green again: https://lab.llvm.org/buildbot/#/builders/172/builds/11594, so this may have been a glitch...

flang/include/flang/Evaluate/constant.h

 class ConstantBounds {
 public:
   ConstantBounds() = default;
   explicit ConstantBounds(const ConstantSubscripts &shape);
   explicit ConstantBounds(ConstantSubscripts &&shape);
   ~ConstantBounds();
   const ConstantSubscripts &shape() const { return shape_; }
   const ConstantSubscripts &lbounds() const { return lbounds_; }
+  ConstantSubscripts ComputeUbounds(std::optional<int> dim) const;
   void set_lbounds(ConstantSubscripts &&);
   void SetLowerBoundsToOne();
   int Rank() const { return GetRank(shape_); }
   Constant<SubscriptInteger> SHAPE() const;
 
   // If no optional dimension order argument is passed, increments a vector of
   // subscripts in Fortran array order (first dimension varying most quickly).
   // Otherwise, increments the vector of subscripts according to the given

flang/lib/Evaluate/constant.cpp

void ConstantBounds::set_lbounds(ConstantSubscripts &&lb) {
   lbounds_ = std::move(lb);
   for (std::size_t j{0}; j < shape_.size(); ++j) {
     if (shape_[j] == 0) {
       lbounds_[j] = 1;
     }
   }
 }
 
+ConstantSubscripts ConstantBounds::ComputeUbounds(
+    std::optional<int> dim) const {
+  if (dim) {
+    CHECK(*dim < Rank());
+    return {lbounds_[*dim] + shape_[*dim] - 1};
+  } else {
+    ConstantSubscripts ubounds(Rank());
+    for (int i = 0; i < Rank(); ++i) {
+      ubounds[i] = lbounds_[i] + shape_[i] - 1;
+    }
+    return ubounds;
+  }
+}
+
 void ConstantBounds::SetLowerBoundsToOne() {
   for (auto &n : lbounds_) {
     n = 1;
   }
 }
 
 Constant<SubscriptInteger> ConstantBounds::SHAPE() const {
   return AsConstantShape(shape_);

flang/lib/Evaluate/fold-integer.cpp

if (asScalar) {
     const int rank = GetRank(bounds);
     std::vector<Scalar<T>> packed(rank);
     std::transform(bounds.begin(), bounds.end(), packed.begin(),
         [](ConstantSubscript x) { return Scalar<T>(x); });
     return Expr<T>{Constant<T>{std::move(packed), ConstantSubscripts{rank}}};
   }
 }
 
-// Class to retrieve the constant lower bound of an expression which is an
+// Class to retrieve the constant bound of an expression which is an
 // array that devolves to a type of Constant<T>
-class GetConstantArrayLboundHelper {
+class GetConstantArrayBoundHelper {
 public:
-  GetConstantArrayLboundHelper(std::optional<ConstantSubscript> dim)
-      : dim_{dim} {}
+  template <typename T>
+  static Expr<T> GetLbound(
+      const Expr<SomeType> &array, std::optional<int> dim) {
+    return PackageConstantBounds<T>(
+        GetConstantArrayBoundHelper(dim, /*getLbound=*/true).Get(array),
+        dim.has_value());
+  }
+
+  template <typename T>
+  static Expr<T> GetUbound(
+      const Expr<SomeType> &array, std::optional<int> dim) {
+    return PackageConstantBounds<T>(
+        GetConstantArrayBoundHelper(dim, /*getLbound=*/false).Get(array),
+        dim.has_value());
+  }
+
+private:
+  GetConstantArrayBoundHelper(
+      std::optional<ConstantSubscript> dim, bool getLbound)
+      : dim_{dim}, getLbound_{getLbound} {}
 
-  template <typename T> ConstantSubscripts GetLbound(const T &) {
+  template <typename T> ConstantSubscripts Get(const T &) {
     // The method is needed for template expansion, but we should never get
     // here in practice.
     CHECK(false);
     return {0};
   }
 
-  template <typename T> ConstantSubscripts GetLbound(const Constant<T> &x) {
+  template <typename T> ConstantSubscripts Get(const Constant<T> &x) {
+    if (getLbound_) {
       // Return the lower bound
       if (dim_) {
         return {x.lbounds().at(*dim_)};
       } else {
         return x.lbounds();
       }
+    } else {
+      return x.ComputeUbounds(dim_);
+    }
   }
 
-  template <typename T> ConstantSubscripts GetLbound(const Parentheses<T> &x) {
+  template <typename T> ConstantSubscripts Get(const Parentheses<T> &x) {
     // LBOUND for (x) is [1, ..., 1] cause of temp variable inside
     // parentheses (lower bound is omitted, the default value is 1).
     return ConstantSubscripts(x.Rank(), ConstantSubscript{1});
   }
 
-  template <typename T> ConstantSubscripts GetLbound(const Expr<T> &x) {
+  template <typename T> ConstantSubscripts Get(const Expr<T> &x) {
     // recurse through Expr<T>'a until we hit a constant
-    return std::visit([&](const auto &inner) { return GetLbound(inner); },
+    return std::visit([&](const auto &inner) { return Get(inner); },
         //      [&](const auto &) { return 0; },
         x.u);
   }
 
-private:
-  std::optional<ConstantSubscript> dim_;
+  const std::optional<ConstantSubscript> dim_;
+  const bool getLbound_;
 };
 
 template <int KIND>
 Expr<Type<TypeCategory::Integer, KIND>> LBOUND(FoldingContext &context,
     FunctionRef<Type<TypeCategory::Integer, KIND>> &&funcRef) {
   using T = Type<TypeCategory::Integer, KIND>;
   ActualArguments &args{funcRef.arguments()};
   if (const auto *array{UnwrapExpr<Expr<SomeType>>(args[0])}) {
if (int rank{array->Rank()}; rank > 0) {
             return Fold(context,
                 ConvertToType<T>(Expr<ExtentType>{std::move(*extents)}));
           }
         } else {
           lowerBoundsAreOne = symbol.Rank() == 0; // LBOUND(array%component)
         }
       }
       if (IsActuallyConstant(*array)) {
-        const ConstantSubscripts bounds =
-            GetConstantArrayLboundHelper{dim}.GetLbound(*array);
-        return PackageConstantBounds<T>(std::move(bounds), dim.has_value());
+        return GetConstantArrayBoundHelper::GetLbound<T>(*array, dim);
       }
       if (lowerBoundsAreOne) {
         ConstantSubscripts ones(rank, ConstantSubscript{1});
         return PackageConstantBounds<T>(std::move(ones), dim.has_value());
       }
     }
   }
   return Expr<T>{std::move(funcRef)};
if (int rank{array->Rank()}; rank > 0) {
               return Fold(context,
                   ConvertToType<T>(Expr<ExtentType>{std::move(*extents)}));
             }
           }
         } else {
           takeBoundsFromShape = symbol.Rank() == 0; // UBOUND(array%component)
         }
       }
+      if (IsActuallyConstant(*array)) {
+        return GetConstantArrayBoundHelper::GetUbound<T>(*array, dim);
+      }
       if (takeBoundsFromShape) {
         if (auto shape{GetContextFreeShape(context, *array)}) {
           if (dim) {
             if (auto &dimSize{shape->at(*dim)}) {
               return Fold(context,
                   ConvertToType<T>(Expr<ExtentType>{std::move(*dimSize)}));
             }
           } else if (auto shapeExpr{AsExtentArrayExpr(*shape)}) {

flang/test/Evaluate/folding08.f90

associate (b => a + 0)
       block
         integer, parameter :: lbb(*) = lbound(b)
         logical, parameter :: test_lbb = all(lbb == [1,1])
         integer, parameter :: ubb(*) = ubound(b)
         logical, parameter :: test_ubb = all(ubb == [2,3])
       end block
     end associate
   end subroutine
-  subroutine test3_lbound_parameter
-    ! Test lbound with constant arrays
+  subroutine test3_bound_parameter
+    ! Test [ul]bound with parameter arrays
     integer, parameter :: a1(1) = 0
     integer, parameter :: lba1(*) = lbound(a1)
     logical, parameter :: test_lba1 = all(lba1 == [1])
+    integer, parameter :: uba1(*) = ubound(a1)
+    logical, parameter :: test_uba1 = all(lba1 == [1])
 
     integer, parameter :: a2(0:1) = 0
     integer, parameter :: lba2(*) = lbound(a2)
     logical, parameter :: test_lba2 = all(lba2 == [0])
+    integer, parameter :: uba2(*) = ubound(a2)
+    logical, parameter :: test_uba2 = all(uba2 == [1])
 
     integer, parameter :: a3(-10:-5,1,4:6) = 0
     integer, parameter :: lba3(*) = lbound(a3)
     logical, parameter :: test_lba3 = all(lba3 == [-10, 1, 4])
+    integer, parameter :: uba3(*) = ubound(a3)
+    logical, parameter :: test_uba3 = all(uba3 == [-5, 1, 6])
+
     ! Exercise with DIM=
     logical, parameter :: test_lba3_dim = lbound(a3, 1) == -10 .and. &
          lbound(a3, 2) == 1 .and. &
          lbound(a3, 3) == 4
+    logical, parameter :: test_uba3_dim = ubound(a3, 1) == -5 .and. &
+         ubound(a3, 2) == 1 .and. &
+         ubound(a3, 3) == 6
   end subroutine
   subroutine test4_lbound_parentheses
     ! Test lbound with (x) expressions
     integer :: a1(1) = 0
     logical, parameter :: test_lba1 = all(lbound((a1)) == [1])
     integer :: a2(0:2) = 0
     logical, parameter :: test_lba2 = all(lbound((a2)) == [1])
     integer :: a3(-1:0) = 0