[flang]Fix incorrect array type transformation

[Leporacanthicus]

Summary

When an array is defined with "unknown" size, such as fir.array<2x?x5xi32>,
it should be converted to llvm.array<10 x i32>. The code so far has
been converting it to llvm.ptr<i32>.

Using a different function to check the if there starting are constant
dimensions, rather than if ALL dimensions are constant, it now produces
the correct array form.

Some tests has been updated, so they are now checking the new behaviour
rather than the old behaviour - so there's no need to add further tests
for this particular scenario.

This was originally found when compiling Spec 17 code, where an assert
in a GepOP was hit. That is bug #56141, which this change fixes.

[kiranchandramohan]

flang/test/Fir/alloc.fir

282

Would this end up over-allocating? There is already a multiplication by 60 (= 4 * 3 *5) for prod1. That seems to have included the 4x portion.

[jeanPerier]

Thanks for fixing the issue

flang/test/Fir/alloc.fir

282

I think it would, yes. @Leporacanthicus, you might need to also update genAllocationScaleSize here: https://github.com/llvm/llvm-project/blob/b094c737cd85b8bf979c3cc998853b03a20d5c59/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L317.

It should probably translate the type to LLVM, and only use the constant extents from the FIR sequence ty[e dimensions that where not kept in the LLVM type.

I think this will remove all usages of hasConstantInterior that was non trivial, so you could delete it then.

[Leporacanthicus]

flang/test/Fir/alloc.fir

282

Thanks for the hint. I've managed to fix one variety, but in the process broken another.

It's not hard to fix, but I think I've got to split the calculation into two parts, one for dynchar arrays, and one for the non-dynchar variety. Or something like that.

I will continue Tuesday (off work on Monday).

[Leporacanthicus]

Updated genAllocationScaleSize and removed now unused hasConstantInterior.
Updated tests to match new allocation scale size (adjusted for the type size being different).

[jeanPerier]

Thanks for cleaning-up the suggestion. This fix helps straighten things out ! A few more suggestions.

flang/lib/Optimizer/CodeGen/CodeGen.cpp

346

Shouldn't that be an else if to avoid counting the extents twice if there was a shape operand ?

Anyway, I am thinking that given the intent of getConstantRows, it should return zero if the sequence element type has a dynamic size. That way I think you could simply have a single if(seqTy && seqTy.getConstantRows() != seqTy.getShape().size()) {...} case.

You would need to update the current hasConstantShape() to not use it anymore because there are usages in lowering that do not care about the element having dynamic size or not (it can become a simple loop on the extents checking if they are constants).

You could rename the current version of hasConstantShape() into hasDynamicSize() for its uses here.

flang/lib/Optimizer/CodeGen/TypeConverter.h

321

With the suggested change, the two ifs could become a single if (!seqTy.hasDynamicSize()) (the current hasConstantShape(), renamed)

349

If getConstantRows is updated as suggested, I think you could also remove that special case about characters here.

359

You would have to use seq.hasDynamicSize() here.

[Leporacanthicus]

Updates based on review comments:

• Rename hasConstantShape to inverse and now called hasDynamicSize
• Return 0 on getConstantRows for dynamicLength character types
• Simplify code as review commetns suggested

[jeanPerier]

Other than my two nits about naming and the fact that fir::hasDynamicType is more generic than characterWithDynamicLen for the future, looks great, thanks !

flang/include/flang/Optimizer/Dialect/FIRTypes.td

463

I am being a bit nitpicky with the name here, but I think this one should be hasDynamicExtents because it does not take into consideration the element type, and I would expect a type containing a fir.char<?> to reply false to hasDynamicSize().

flang/lib/Optimizer/Dialect/FIRType.cpp

786

fir::hasDynamicType is more generic than characterWithDynamicLen, it will account for the future parametrized derived types when they get there.

[Leporacanthicus]

Update the newly added function to hasDynamicExtents instead of the
already used hasDynamicSize.

Change the "is variable length" to use hasDynamicSize (the original
function) instead of restricting it to characterWithDynamicLen.

[jeanPerier]

Thanks @Leporacanthicus, that looks good to me.

flang/include/flang/Lower/SymbolMap.h

return match(
         [](const Box &arr) { return arr.getLBounds().empty(); },
         [](const auto &) { return false; });
   }
 
   /// Does the boxed value have a constant shape?
   bool hasConstantShape() const {
     if (auto eleTy = fir::dyn_cast_ptrEleTy(getAddr().getType()))
       if (auto arrTy = eleTy.dyn_cast<fir::SequenceType>())
-        return arrTy.hasConstantShape();
+        return !arrTy.hasDynamicExtents();
     return false;
   }
 
   /// Get the lbound if the box explicitly contains it.
   mlir::Value getLBound(unsigned dim) const {
     return match([&](const FullDim &box) { return box.getLBounds()[dim]; },
                  [&](const CharFullDim &box) { return box.getLBounds()[dim]; },
                  [&](const Box &box) { return box.getLBounds()[dim]; },

flang/include/flang/Optimizer/Dialect/FIRType.h

inline bool characterWithDynamicLen(mlir::Type t) {
   if (auto charTy = t.dyn_cast<fir::CharacterType>())
     return charTy.hasDynamicLen();
   return false;
 }
 
 /// Returns true iff `seqTy` has either an unknown shape or a non-constant shape
 /// (where rank > 0).
 inline bool sequenceWithNonConstantShape(fir::SequenceType seqTy) {
-  return seqTy.hasUnknownShape() || !seqTy.hasConstantShape();
+  return seqTy.hasUnknownShape() || seqTy.hasDynamicExtents();
 }
 
 /// Returns true iff the type `t` does not have a constant size.
 bool hasDynamicSize(mlir::Type t);
 
 inline unsigned getRankOfShapeType(mlir::Type t) {
   if (auto shTy = t.dyn_cast<fir::ShapeType>())
     return shTy.getRank();

flang/include/flang/Optimizer/Dialect/FIRTypes.td

let extraClassDeclaration = [{
     using Extent = int64_t;
     using Shape = llvm::SmallVector<Extent>;
     using ShapeRef = llvm::ArrayRef<int64_t>;
     unsigned getConstantRows() const;
 
     // The number of dimensions of the sequence
     unsigned getDimension() const { return getShape().size(); }
 
-    // Is the interior of the sequence constant? Check if the array is
-    // one of constant shape (`array<C...xCxT>`), unknown shape
-    // (`array<*xT>`), or rows with shape and ending with column(s) of
-    // unknown extent (`array<C...xCx?...x?xT>`).
-    bool hasConstantInterior() const;
-
-    // Is the shape of the sequence constant?
-    bool hasConstantShape() const {
-      return getConstantRows() == getDimension();
+    // Is the shape of the sequence dynamic?
+    bool hasDynamicExtents() const {
+      for(const auto d : getShape())
+        if (d == getUnknownExtent())
+          return true;
+      return false;
     }
 
     // Does the sequence have unknown shape? (`array<* x T>`)
     bool hasUnknownShape() const { return getShape().empty(); }
 
     // The value `-1` represents an unknown extent for a dimension
     static constexpr Extent getUnknownExtent() { return -1; }
   }];

flang/lib/Lower/ConvertExpr.cpp

if (auto charTy = seqTy.getEleTy().dyn_cast<fir::CharacterType>())
         TODO(loc, "character array expression temp with dynamic length");
     if (auto recTy = seqTy.getEleTy().dyn_cast<fir::RecordType>())
       if (recTy.getNumLenParams() > 0)
         TODO(loc, "derived type array expression temp with LEN parameters");
     if (mlir::Type eleTy = fir::unwrapSequenceType(type);
         fir::isRecordWithAllocatableMember(eleTy))
       TODO(loc, "creating an array temp where the element type has "
                 "allocatable members");
-    mlir::Value temp = seqTy.hasConstantShape()
+    mlir::Value temp = !seqTy.hasDynamicExtents()
                            ? builder.create<fir::AllocMemOp>(loc, type)
                            : builder.create<fir::AllocMemOp>(
                                  loc, type, ".array.expr", llvm::None, shape);
     fir::FirOpBuilder *bldr = &converter.getFirOpBuilder();
     stmtCtx.attachCleanup(
         [bldr, loc, temp]() { bldr->create<fir::FreeMemOp>(loc, temp); });
     mlir::Value shapeOp = genShapeOp(shape);
     return builder.create<fir::ArrayLoadOp>(loc, seqTy, temp, shapeOp,

flang/lib/Optimizer/CodeGen/CodeGen.cpp

 // dynamic length arrays, since those are the only ones that get decayed to a
 // pointer to the element type.
 template <typename OP>
 static mlir::Value
 genAllocationScaleSize(OP op, mlir::Type ity,
                        mlir::ConversionPatternRewriter &rewriter) {
   mlir::Location loc = op.getLoc();
   mlir::Type dataTy = op.getInType();
-  mlir::Type scalarType = fir::unwrapSequenceType(dataTy);
   auto seqTy = dataTy.dyn_cast<fir::SequenceType>();
-  if ((op.hasShapeOperands() && seqTy && !seqTy.hasConstantInterior()) ||
-      (seqTy && fir::characterWithDynamicLen(scalarType))) {
-    fir::SequenceType::Extent constSize = 1;
-    for (auto extent : seqTy.getShape())
+  if (seqTy) {
+    int constRows = seqTy.getConstantRows();
+    const fir::SequenceType::ShapeRef &shape = seqTy.getShape();
+    if (constRows != static_cast<int>(shape.size())) {
+      for (auto extent : shape) {
+        if (constRows-- > 0)
+          continue;
         if (extent != fir::SequenceType::getUnknownExtent())
           constSize *= extent;
-    if (constSize != 1) {
-      mlir::Value constVal{
-          genConstantIndex(loc, ity, rewriter, constSize).getResult()};
-      return constVal;
-    }
-  }
+      }
+    }
+  }
+
   return nullptr;
 }
 
 namespace {
 /// convert to LLVM IR dialect `alloca`
 struct AllocaOpConversion : public FIROpConversion<fir::AllocaOp> {
   using FIROpConversion::FIROpConversion;
 

flang/lib/Optimizer/CodeGen/PreCGRewrite.cpp

public:
   mlir::LogicalResult
   matchAndRewrite(fir::EmboxOp embox,
                   mlir::PatternRewriter &rewriter) const override {
     // If the embox does not include a shape, then do not convert it
     if (auto shapeVal = embox.getShape())
       return rewriteDynamicShape(embox, rewriter, shapeVal);
     if (auto boxTy = embox.getType().dyn_cast<fir::BoxType>())
       if (auto seqTy = boxTy.getEleTy().dyn_cast<fir::SequenceType>())
-        if (seqTy.hasConstantShape())
+        if (!seqTy.hasDynamicExtents())
           return rewriteStaticShape(embox, rewriter, seqTy);
     return mlir::failure();
   }
 
   mlir::LogicalResult rewriteStaticShape(fir::EmboxOp embox,
                                          mlir::PatternRewriter &rewriter,
                                          fir::SequenceType seqTy) const {
     auto loc = embox.getLoc();

flang/lib/Optimizer/CodeGen/TypeConverter.h

public:
   template <typename A>
   mlir::Type convertPointerLike(A &ty) {
     mlir::Type eleTy = ty.getEleTy();
     // A sequence type is a special case. A sequence of runtime size on its
     // interior dimensions lowers to a memory reference. In that case, we
     // degenerate the array and do not want a the type to become `T**` but
     // merely `T*`.
     if (auto seqTy = eleTy.dyn_cast<fir::SequenceType>()) {
-      if (!seqTy.hasConstantShape() ||
+      if (seqTy.hasDynamicExtents() ||
           characterWithDynamicLen(seqTy.getEleTy())) {
-        if (seqTy.hasConstantInterior())
+        if (seqTy.getConstantRows() > 0)
           return convertType(seqTy);
         eleTy = seqTy.getEleTy();
       }
     }
     // fir.ref<fir.box> is a special case because fir.box type is already
     // a pointer to a Fortran descriptor at the LLVM IR level. This implies
     // that a fir.ref<fir.box>, that is the address of fir.box is actually
     // the same as a fir.box at the LLVM level.
mlir::Type convertRealType(fir::KindTy kind) {
     return fir::fromRealTypeID(&getContext(), kindMapping.getRealTypeID(kind),
                                kind);
   }
 
   // fir.array<c ... :any>  -->  llvm<"[...[c x any]]">
   mlir::Type convertSequenceType(SequenceType seq) {
     auto baseTy = convertType(seq.getEleTy());
     if (characterWithDynamicLen(seq.getEleTy()))
       return mlir::LLVM::LLVMPointerType::get(baseTy);
     auto shape = seq.getShape();
     auto constRows = seq.getConstantRows();
     if (constRows) {
       decltype(constRows) i = constRows;
       for (auto e : shape) {
         baseTy = mlir::LLVM::LLVMArrayType::get(baseTy, e);
         if (--i == 0)
           break;
       }
-      if (seq.hasConstantShape())
+      if (!seq.hasDynamicExtents())
         return baseTy;
     }
     return mlir::LLVM::LLVMPointerType::get(baseTy);
   }
 
   // fir.tdesc<any>  -->  llvm<"i8*">
   // TODO: For now use a void*, however pointer identity is not sufficient for
   // the f18 object v. class distinction (F2003).

flang/lib/Optimizer/Dialect/FIRType.cpp

void fir::SequenceType::print(mlir::AsmPrinter &printer) const {
   if (auto map = getLayoutMap()) {
     printer << ", ";
     map.print(printer.getStream());
   }
   printer << '>';
 }
 
 unsigned fir::SequenceType::getConstantRows() const {
+  if (hasDynamicSize(getEleTy()))
+    return 0;
   auto shape = getShape();
   unsigned count = 0;
   for (auto d : shape) {
-    if (d < 0)
+    if (d == getUnknownExtent())
       break;
     ++count;
   }
   return count;
 }
 
-// This test helps us determine if we can degenerate an array to a
-// pointer to some interior section (possibly a single element) of the
-// sequence. This is used to determine if we can lower to the LLVM IR.
-bool fir::SequenceType::hasConstantInterior() const {
-  if (hasUnknownShape())
-    return true;
-  auto rows = getConstantRows();
-  auto dim = getDimension();
-  if (rows == dim)
-    return true;
-  auto shape = getShape();
-  for (unsigned i = rows, size = dim; i < size; ++i)
-    if (shape[i] != getUnknownExtent())
-      return false;
-  return true;
-}
-
 mlir::LogicalResult fir::SequenceType::verify(
     llvm::function_ref<mlir::InFlightDiagnostic()> emitError,
     llvm::ArrayRef<int64_t> shape, mlir::Type eleTy,
     mlir::AffineMapAttr layoutMap) {
   // DIMENSION attribute can only be applied to an intrinsic or record type
   if (eleTy.isa<BoxType, BoxCharType, BoxProcType, ShapeType, ShapeShiftType,
                 ShiftType, SliceType, FieldType, LenType, HeapType, PointerType,
                 ReferenceType, TypeDescType, fir::VectorType, SequenceType>())

flang/test/Fir/alloc.fir

 // CHECK: call ptr @malloc(i64 %[[prod3]])
 func.func @allocmem_dynarray_of_dynchar2(%l: i32, %e : index) -> !fir.heap<!fir.array<?x?x!fir.char<2,?>>> {
   %1 = fir.allocmem !fir.array<?x?x!fir.char<2,?>>(%l : i32), %e, %e
   return %1 : !fir.heap<!fir.array<?x?x!fir.char<2,?>>>
 }
 
 // CHECK-LABEL: define ptr @alloca_array_with_holes_nonchar(
 // CHECK-SAME: i64 %[[a:.*]], i64 %[[b:.*]])
-// CHECK: %[[prod1:.*]] = mul i64 60, %[[a]]
+// CHECK: %[[prod1:.*]] = mul i64 15, %[[a]]
 // CHECK: %[[prod2:.*]] = mul i64 %[[prod1]], %[[b]]
-// CHECK: alloca i32, i64 %[[prod2]]
+// CHECK: alloca [4 x i32], i64 %[[prod2]]
 func.func @alloca_array_with_holes_nonchar(%0 : index, %1 : index) -> !fir.ref<!fir.array<4x?x3x?x5xi32>> {
   %a = fir.alloca !fir.array<4x?x3x?x5xi32>, %0, %1
   return %a : !fir.ref<!fir.array<4x?x3x?x5xi32>>
 }
 
 // CHECK-LABEL: define ptr @alloca_array_with_holes_char(
 // CHECK-SAME: i64 %[[e:.*]])
-// CHECK: %[[mul:.*]] = mul i64 12, %[[e]]
-// CHECK: alloca [10 x i16], i64 %[[mul]]
+// CHECK: %[[mul:.*]] = mul i64 4, %[[e]]
+// CHECK: alloca [3 x [10 x i16]], i64 %[[mul]]
 func.func @alloca_array_with_holes_char(%e: index) -> !fir.ref<!fir.array<3x?x4x!fir.char<2,10>>> {
   %1 = fir.alloca !fir.array<3x?x4x!fir.char<2,10>>, %e
   return %1 : !fir.ref<!fir.array<3x?x4x!fir.char<2,10>>>
 }
 
 // CHECK-LABEL: define ptr @alloca_array_with_holes_dynchar(
 // CHECK-SAME: i64 %[[len:.*]], i64 %[[extent:.*]])
 // CHECK: %[[a:.*]] = mul i64 %[[len]], 12
 // CHECK: %[[b:.*]] = mul i64 %[[a]], %[[extent]]
 // CHECK: alloca i16, i64 %[[b]]
 func.func @alloca_array_with_holes_dynchar(%arg0: index, %arg1: index) -> !fir.ref<!fir.array<3x?x4x!fir.char<2,?>>> {
   %1 = fir.alloca !fir.array<3x?x4x!fir.char<2,?>>(%arg0 : index), %arg1
   return %1 : !fir.ref<!fir.array<3x?x4x!fir.char<2,?>>>
 }
 
 // CHECK-LABEL: define ptr @allocmem_array_with_holes_nonchar(
 // CHECK-SAME: i64 %[[e1:.*]], i64 %[[e2:.*]])
-// CHECK: %[[a:.*]] = mul i64 240, %[[e1]]
+// CHECK: %[[a:.*]] = mul i64 mul (i64 ptrtoint{{.*}} 15), %[[e1]]
 // CHECK: %[[b:.*]] = mul i64 %3, %[[e2]]
 // CHECK: call ptr @malloc(i64 %[[b]])
 func.func @allocmem_array_with_holes_nonchar(%0 : index, %1 : index) -> !fir.heap<!fir.array<4x?x3x?x5xi32>> {
   %a = fir.allocmem !fir.array<4x?x3x?x5xi32>, %0, %1
   return %a : !fir.heap<!fir.array<4x?x3x?x5xi32>>
 }
 
 // CHECK-LABEL: define ptr @allocmem_array_with_holes_char(
 // CHECK-SAME: i64 %[[e:.*]])
-// CHECK: %[[mul:.*]] = mul i64 mul (i64 ptrtoint (ptr getelementptr ([10 x i16], ptr null, i64 1) to i64), i64 12), %[[e]]
+// CHECK: %[[mul:.*]] = mul i64 mul (i64 ptrtoint (ptr getelementptr ([3 x [10 x i16]], ptr null, i64 1) to i64), i64 4), %[[e]]
 // CHECK: call ptr @malloc(i64 %[[mul]])
 func.func @allocmem_array_with_holes_char(%e: index) -> !fir.heap<!fir.array<3x?x4x!fir.char<2,10>>> {
   %1 = fir.allocmem !fir.array<3x?x4x!fir.char<2,10>>, %e
   return %1 : !fir.heap<!fir.array<3x?x4x!fir.char<2,10>>>
 }
 
 // CHECK-LABEL: define ptr @allocmem_array_with_holes_dynchar(
 // CHECK-SAME: i64 %[[len:.*]], i64 %[[extent:.*]])
 // CHECK: %[[a:.*]] = mul i64 24, %[[len]]
 // CHECK: %[[b:.*]] = mul i64 %[[a]], %[[extent]]
 // CHECK: call ptr @malloc(i64 %[[b]])
 func.func @allocmem_array_with_holes_dynchar(%arg0: index, %arg1: index) -> !fir.heap<!fir.array<3x?x4x!fir.char<2,?>>> {
   %1 = fir.allocmem !fir.array<3x?x4x!fir.char<2,?>>(%arg0 : index), %arg1
   return %1 : !fir.heap<!fir.array<3x?x4x!fir.char<2,?>>>
 }
 

flang/test/Fir/convert-to-llvm.fir

 // Constant factor of 60 (4*3*5) must be included.
 
 func.func @alloca_array_with_holes(%0 : index, %1 : index) -> !fir.ref<!fir.array<4x?x3x?x5xi32>> {
   %a = fir.alloca !fir.array<4x?x3x?x5xi32>, %0, %1
   return %a : !fir.ref<!fir.array<4x?x3x?x5xi32>>
 }
 
 // CHECK-LABEL: llvm.func @alloca_array_with_holes
-// CHECK-SAME: ([[A:%.*]]: i64, [[B:%.*]]: i64) -> !llvm.ptr<i32>
+// CHECK-SAME: ([[A:%.*]]: i64, [[B:%.*]]: i64) -> !llvm.ptr<array<4 x i32>>
 // CHECK-DAG: [[ONE:%.*]] = llvm.mlir.constant(1 : i64) : i64
-// CHECK-DAG: [[FIXED:%.*]] = llvm.mlir.constant(60 : i64) : i64
+// CHECK-DAG: [[FIXED:%.*]] = llvm.mlir.constant(15 : i64) : i64
 // CHECK: [[PROD1:%.*]] = llvm.mul [[ONE]], [[FIXED]] : i64
 // CHECK: [[PROD2:%.*]] = llvm.mul [[PROD1]], [[A]] : i64
 // CHECK: [[PROD3:%.*]] = llvm.mul [[PROD2]], [[B]] : i64
-// CHECK: [[RES:%.*]] = llvm.alloca [[PROD3]] x i32 {in_type = !fir.array<4x?x3x?x5xi32>
-// CHECK: llvm.return [[RES]] : !llvm.ptr<i32>
+// CHECK: [[RES:%.*]] = llvm.alloca [[PROD3]] x !llvm.array<4 x i32> {in_type = !fir.array<4x?x3x?x5xi32>
+// CHECK: llvm.return [[RES]] : !llvm.ptr<array<4 x i32>>
 
 // -----
 
 // Test `fir.select_case` operation conversion with INTEGER.
 
 func.func @select_case_integer(%arg0: !fir.ref<i32>) -> i32 {
   %2 = fir.load %arg0 : !fir.ref<i32>
   %c1_i32 = arith.constant 1 : i32

flang/test/Fir/types-to-llvm.fir

 func.func private @foo1(%arg0: !fir.ref<!fir.array<10xf32>>)
 // CHECK-LABEL: foo1
 // CHECK-SAME: !llvm.ptr<array<10 x f32>>
 func.func private @foo2(%arg0: !fir.ref<!fir.box<!fir.heap<!fir.type<_QMs1Ta1{x:i32,y:f32}>>>>)
 // CHECK-LABEL: foo2
 // CHECK-SAME: !llvm.ptr<struct<(ptr<struct<"_QMs1Ta1", (i32, f32)>>, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, ptr<i{{.*}}>, array<1 x i{{.*}}>)>>
 func.func private @foo3(%arg0: !fir.ref<!fir.array<10x?x11xf32>>)
 // CHECK-LABEL: foo3
-// CHECK-SAME: !llvm.ptr<f32>
+// CHECK-SAME: !llvm.ptr<array<10 x f32>>
 func.func private @foo4(%arg0: !fir.ref<!fir.array<10x11x?x?xf32>>)
 // CHECK-LABEL: foo4
 // CHECK-SAME: !llvm.ptr<array<11 x array<10 x f32>>>
 func.func private @foo5(%arg0: !fir.ref<!fir.array<10x!fir.char<1,?>>>)
 // CHECK-LABEL: foo5
 // CHECK-SAME: !llvm.ptr<i8>
 func.func private @foo6(%arg0: !fir.ref<!fir.array<10x!fir.char<2,8>>>)
 // CHECK-LABEL: foo6