[flang] Fix lowering issue with character temp

[clementval]

Summary

• Add verifiers that determine if an Op requires type parameters or not and checks that the correct number of parameters is specified.

This patch is part of the upstreaming effort from fir-dev branch.

Co-authored-by: Eric Schweitz <[e-mail]>

[clementval]

Restore runline and unsupported system

[PeteSteinfeld]

I get a build error for this change. Here's what I see in the log file:

[5929/6618] Building CXX object tools/flang/lib/Optimizer/Dialect/CMakeFiles/obj.FIRDialect.dir/FIROps.cpp.o
FAILED: tools/flang/lib/Optimizer/Dialect/CMakeFiles/obj.FIRDialect.dir/FIROps.cpp.o 
/home/sw/thirdparty/gcc/gcc-9.3.0/linux86-64/bin/g++ -DFLANG_LITTLE_ENDIAN=1 -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/local/home/psteinfeld/main/temp/build/tools/flang/lib/Optimizer/Dialect -I/local/home/psteinfeld/main/temp/flang/lib/Optimizer/Dialect -I/local/home/psteinfeld/main/temp/flang/include -I/local/home/psteinfeld/main/temp/build/tools/flang/include -I/local/home/psteinfeld/main/temp/build/include -I/local/home/psteinfeld/main/temp/llvm/include -isystem /local/home/psteinfeld/main/temp/llvm/../mlir/include -isystem /local/home/psteinfeld/main/temp/build/tools/mlir/include -isystem /local/home/psteinfeld/main/temp/build/tools/clang/include -isystem /local/home/psteinfeld/main/temp/llvm/../clang/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -Werror -Wno-deprecated-copy -fno-strict-aliasing -fno-semantic-interposition -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/flang/lib/Optimizer/Dialect/CMakeFiles/obj.FIRDialect.dir/FIROps.cpp.o -MF tools/flang/lib/Optimizer/Dialect/CMakeFiles/obj.FIRDialect.dir/FIROps.cpp.o.d -o tools/flang/lib/Optimizer/Dialect/CMakeFiles/obj.FIRDialect.dir/FIROps.cpp.o -c /local/home/psteinfeld/main/temp/flang/lib/Optimizer/Dialect/FIROps.cpp
/local/home/psteinfeld/main/temp/flang/lib/Optimizer/Dialect/FIROps.cpp: In static member function ‘static mlir::ParseResult fir::CoordinateOp::parse(mlir::OpAsmParser&, mlir::OperationState&)’:
/local/home/psteinfeld/main/temp/flang/lib/Optimizer/Dialect/FIROps.cpp:952:58: error: ignoring returned value of type ‘mlir::ParseResult’, declared with attribute nodiscard [-Werror=unused-result]
  952 |   parser.addTypesToList(funcTy.getResults(), result.types);
      |                                                          ^
In file included from /local/home/psteinfeld/main/temp/mlir/include/mlir/Dialect/Arithmetic/IR/Arithmetic.h:13,
                 from /local/home/psteinfeld/main/temp/flang/include/flang/Optimizer/Dialect/FIROps.h:13,
                 from /local/home/psteinfeld/main/temp/flang/lib/Optimizer/Dialect/FIROps.cpp:13:
/local/home/psteinfeld/main/temp/mlir/include/mlir/IR/OpImplementation.h:1002:15: note: in call to ‘mlir::ParseResult mlir::AsmParser::addTypesToList(llvm::ArrayRef<mlir::Type>, llvm::SmallVectorImpl<mlir::Type>&)’, declared here
 1002 |   ParseResult addTypesToList(ArrayRef<Type> types,
      |               ^~~~~~~~~~~~~~
In file included from /local/home/psteinfeld/main/temp/mlir/include/mlir/IR/StorageUniquerSupport.h:17,
                 from /local/home/psteinfeld/main/temp/mlir/include/mlir/IR/AttributeSupport.h:17,
                 from /local/home/psteinfeld/main/temp/mlir/include/mlir/IR/Attributes.h:12,
                 from /local/home/psteinfeld/main/temp/mlir/include/mlir/IR/BuiltinAttributeInterfaces.h:13,
                 from /local/home/psteinfeld/main/temp/mlir/include/mlir/IR/BuiltinAttributes.h:12,
                 from /local/home/psteinfeld/main/temp/flang/include/flang/Optimizer/Dialect/FIRType.h:16,
                 from /local/home/psteinfeld/main/temp/flang/include/flang/Optimizer/Dialect/FIROps.h:12,
                 from /local/home/psteinfeld/main/temp/flang/lib/Optimizer/Dialect/FIROps.cpp:13:
/local/home/psteinfeld/main/temp/mlir/include/mlir/Support/LogicalResult.h:112:22: note: ‘mlir::ParseResult’ declared here
  112 | class LLVM_NODISCARD ParseResult : public LogicalResult {
      |                      ^~~~~~~~~~~
cc1plus: all warnings being treated as errors
[5930/6618] Building CXX object tools/mlir/lib/Dialect/LLVMIR/CMakeFiles/obj.MLIRLLVMDialect.dir/IR/LLVMDialect.cpp.o

[PeteSteinfeld]

In my previous comments, I mean to request changes!

[clementval]

Fix warnings

[clementval]

In D128828#3619451, @PeteSteinfeld wrote:

In my previous comments, I mean to request changes!

Should be fixed.

[clementval]

Restore run lines for tests

[PeteSteinfeld]

Thanks, Valentin!

All builds and tests correctly and looks good.

flang/lib/Lower/ConvertExpr.cpp

for (auto [fst, snd] :
         auto boxProcTy = builder.getBoxProcType(funcTy);
         if (mlir::Value host = argumentHostAssocs(converter, fst)) {
           cast = builder.create<fir::EmboxProcOp>(
               loc, boxProcTy, llvm::ArrayRef<mlir::Value>{fst, host});
         } else {
           cast = builder.create<fir::EmboxProcOp>(loc, boxProcTy, fst);
         }
       } else {
+        if (fir::isa_derived(snd)) {
+          // FIXME: This seems like a serious bug elsewhere in lowering. Paper
+          // over the problem for now.
+          TODO(loc, "derived type argument passed by value");
+        }
+        assert(!fir::isa_derived(snd));
         cast = builder.convertWithSemantics(loc, snd, fst,
                                             callingImplicitInterface);
       }
       operands.push_back(cast);
     }
 
     // Add host associations as necessary.
     if (addHostAssociations)
private:
   fir::ArrayLoadOp
   createAndLoadSomeArrayTemp(mlir::Type type,
                              llvm::ArrayRef<mlir::Value> shape) {
     if (ccLoadDest.hasValue())
       return ccLoadDest.getValue()(shape);
     auto seqTy = type.dyn_cast<fir::SequenceType>();
     assert(seqTy && "must be an array");
     mlir::Location loc = getLoc();
-    // TODO: Need to thread the length parameters here. For character, they may
+    // TODO: Need to thread the LEN parameters here. For character, they may
     // differ from the operands length (e.g concatenation). So the array loads
     // type parameters are not enough.
     if (auto charTy = seqTy.getEleTy().dyn_cast<fir::CharacterType>())
       if (charTy.hasDynamicLen())
         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");
#define GENBIN(GenBinEvOp, GenBinTyCat, GenBinFirOp)                           \
   /// size is not constant.
   mlir::Value computeElementSize(const ExtValue &exv, mlir::Type eleTy,
                                  mlir::Type resTy) {
     mlir::Location loc = getLoc();
     mlir::IndexType idxTy = builder.getIndexType();
     mlir::Value multiplier = builder.createIntegerConstant(loc, idxTy, 1);
     if (fir::hasDynamicSize(eleTy)) {
       if (auto charTy = eleTy.dyn_cast<fir::CharacterType>()) {
-        // Array of char with dynamic length parameter. Downcast to an array
+        // Array of char with dynamic LEN parameter. Downcast to an array
         // of singleton char, and scale by the len type parameter from
         // `exv`.
         exv.match(
             [&](const fir::CharBoxValue &cb) { multiplier = cb.getLen(); },
             [&](const fir::CharArrayBoxValue &cb) { multiplier = cb.getLen(); },
             [&](const fir::BoxValue &box) {
               multiplier = fir::factory::CharacterExprHelper(builder, loc)
                                .readLengthFromBox(box.getAddr());

flang/lib/Optimizer/CodeGen/CodeGen.cpp

doRewrite(fir::cg::XArrayCoorOp coor, mlir::Type ty, OpAdaptor adaptor,
     if (coor.subcomponent().empty()) {
       // No subcomponent.
       if (!coor.lenParams().empty()) {
         // Type parameters. Adjust element size explicitly.
         auto eleTy = fir::dyn_cast_ptrEleTy(coor.getType());
         assert(eleTy && "result must be a reference-like type");
         if (fir::characterWithDynamicLen(eleTy)) {
           assert(coor.lenParams().size() == 1);
-          auto bitsInChar = lowerTy().getKindMap().getCharacterBitsize(
-              eleTy.cast<fir::CharacterType>().getFKind());
-          auto scaling = genConstantIndex(loc, idxTy, rewriter, bitsInChar / 8);
-          auto scaledBySize =
-              rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, offset, scaling);
-          auto length =
-              integerCast(loc, rewriter, idxTy,
-                          adaptor.getOperands()[coor.lenParamsOffset()]);
-          offset = rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, scaledBySize,
-                                                      length);
+          auto length = integerCast(loc, rewriter, idxTy,
+                                    operands[coor.lenParamsOffset()]);
+          offset =
+              rewriter.create<mlir::LLVM::MulOp>(loc, idxTy, offset, length);
+
         } else {
           TODO(loc, "compute size of derived type with type parameters");
         }
       }
       // Cast the base address to a pointer to T.
       base = rewriter.create<mlir::LLVM::BitcastOp>(loc, ty,
                                                     adaptor.getOperands()[0]);
     } else {

flang/lib/Optimizer/Dialect/FIROps.cpp

if (fir::isa_unknown_size_box(fir::dyn_cast_ptrEleTy(outType)))
     return emitOpError("cannot allocate !fir.box of unknown rank or type");
   return mlir::success();
 }
 
 //===----------------------------------------------------------------------===//
 // ArrayCoorOp
 //===----------------------------------------------------------------------===//
 
+// CHARACTERs and derived types with LEN PARAMETERs are dependent types that
+// require runtime values to fully define the type of an object.
+static bool validTypeParams(mlir::Type dynTy, mlir::ValueRange typeParams) {
+  dynTy = fir::unwrapAllRefAndSeqType(dynTy);
+  // A box value will contain type parameter values itself.
+  if (dynTy.isa<fir::BoxType>())
+    return typeParams.size() == 0;
+  // Derived type must have all type parameters satisfied.
+  if (auto recTy = dynTy.dyn_cast<fir::RecordType>())
+    return typeParams.size() == recTy.getNumLenParams();
+  // Characters with non-constant LEN must have a type parameter value.
+  if (auto charTy = dynTy.dyn_cast<fir::CharacterType>())
+    if (charTy.hasDynamicLen())
+      return typeParams.size() == 1;
+  // Otherwise, any type parameters are invalid.
+  return typeParams.size() == 0;
+}
+
 mlir::LogicalResult fir::ArrayCoorOp::verify() {
   auto eleTy = fir::dyn_cast_ptrOrBoxEleTy(getMemref().getType());
   auto arrTy = eleTy.dyn_cast<fir::SequenceType>();
   if (!arrTy)
     return emitOpError("must be a reference to an array");
   auto arrDim = arrTy.getDimension();
 
   if (auto shapeOp = getShape()) {
mlir::LogicalResult fir::ArrayCoorOp::verify() {
   if (auto sliceOp = getSlice()) {
     if (auto sl = mlir::dyn_cast_or_null<fir::SliceOp>(sliceOp.getDefiningOp()))
       if (!sl.getSubstr().empty())
         return emitOpError("array_coor cannot take a slice with substring");
     if (auto sliceTy = sliceOp.getType().dyn_cast<fir::SliceType>())
       if (sliceTy.getRank() != arrDim)
         return emitOpError("rank of dimension in slice mismatched");
   }
+  if (!validTypeParams(getMemref().getType(), getTypeparams()))
+    return emitOpError("invalid type parameters");
 
   return mlir::success();
 }
 
 //===----------------------------------------------------------------------===//
 // ArrayLoadOp
 //===----------------------------------------------------------------------===//
 
if (auto sliceOp = getSlice()) {
     if (auto sl = mlir::dyn_cast_or_null<fir::SliceOp>(sliceOp.getDefiningOp()))
       if (!sl.getSubstr().empty())
         return emitOpError("array_load cannot take a slice with substring");
     if (auto sliceTy = sliceOp.getType().dyn_cast<fir::SliceType>())
       if (sliceTy.getRank() != arrDim)
         return emitOpError("rank of dimension in slice mismatched");
   }
 
+  if (!validTypeParams(getMemref().getType(), getTypeparams()))
+    return emitOpError("invalid type parameters");
+
   return mlir::success();
 }
 
 //===----------------------------------------------------------------------===//
 // ArrayMergeStoreOp
 //===----------------------------------------------------------------------===//
 
 mlir::LogicalResult fir::ArrayMergeStoreOp::verify() {
if (auto sl = getSlice()) {
     }
     return mlir::success();
   }
   auto eleTy = fir::dyn_cast_ptrOrBoxEleTy(getMemref().getType());
   if (getOriginal().getType() != eleTy)
     return emitOpError("type of origin does not match memref element type");
   if (getSequence().getType() != eleTy)
     return emitOpError("type of sequence does not match memref element type");
+  if (!validTypeParams(getMemref().getType(), getTypeparams()))
+    return emitOpError("invalid type parameters");
   return mlir::success();
 }
 
 //===----------------------------------------------------------------------===//
 // ArrayFetchOp
 //===----------------------------------------------------------------------===//
 
 // Template function used for both array_fetch and array_update verification.
mlir::LogicalResult fir::ArrayFetchOp::verify() {
   if (indSize == arrTy.getDimension() &&
       ::adjustedElementType(getElement().getType()) != arrTy.getEleTy())
     return emitOpError("return type does not match array");
   auto ty = validArraySubobject(*this);
   if (!ty || ty != ::adjustedElementType(getType()))
     return emitOpError("return type and/or indices do not type check");
   if (!mlir::isa<fir::ArrayLoadOp>(getSequence().getDefiningOp()))
     return emitOpError("argument #0 must be result of fir.array_load");
+  if (!validTypeParams(arrTy, getTypeparams()))
+    return emitOpError("invalid type parameters");
   return mlir::success();
 }
 
 //===----------------------------------------------------------------------===//
 // ArrayAccessOp
 //===----------------------------------------------------------------------===//
 
 mlir::LogicalResult fir::ArrayAccessOp::verify() {
   auto arrTy = getSequence().getType().cast<fir::SequenceType>();
   std::size_t indSize = getIndices().size();
   if (indSize < arrTy.getDimension())
     return emitOpError("number of indices != dimension of array");
   if (indSize == arrTy.getDimension() &&
       getElement().getType() != fir::ReferenceType::get(arrTy.getEleTy()))
     return emitOpError("return type does not match array");
   mlir::Type ty = validArraySubobject(*this);
   if (!ty || fir::ReferenceType::get(ty) != getType())
     return emitOpError("return type and/or indices do not type check");
+  if (!validTypeParams(arrTy, getTypeparams()))
+    return emitOpError("invalid type parameters");
   return mlir::success();
 }
 
 //===----------------------------------------------------------------------===//
 // ArrayUpdateOp
 //===----------------------------------------------------------------------===//
 
 mlir::LogicalResult fir::ArrayUpdateOp::verify() {
   if (fir::isa_ref_type(getMerge().getType()))
     return emitOpError("does not support reference type for merge");
   auto arrTy = getSequence().getType().cast<fir::SequenceType>();
   auto indSize = getIndices().size();
   if (indSize < arrTy.getDimension())
     return emitOpError("number of indices != dimension of array");
   if (indSize == arrTy.getDimension() &&
       ::adjustedElementType(getMerge().getType()) != arrTy.getEleTy())
     return emitOpError("merged value does not have element type");
   auto ty = validArraySubobject(*this);
   if (!ty || ty != ::adjustedElementType(getMerge().getType()))
     return emitOpError("merged value and/or indices do not type check");
+  if (!validTypeParams(arrTy, getTypeparams()))
+    return emitOpError("invalid type parameters");
   return mlir::success();
 }
 
 //===----------------------------------------------------------------------===//
 // ArrayModifyOp
 //===----------------------------------------------------------------------===//
 
 mlir::LogicalResult fir::ArrayModifyOp::verify() {
void fir::CallOp::build(mlir::OpBuilder &builder, mlir::OperationState &result,
                         mlir::ValueRange operands) {
   result.addOperands(operands);
   if (callee)
     result.addAttribute(getCalleeAttrNameStr(), callee);
   result.addTypes(results);
 }
 
 //===----------------------------------------------------------------------===//
+// CharConvertOp
+//===----------------------------------------------------------------------===//
+
+mlir::LogicalResult fir::CharConvertOp::verify() {
+  auto unwrap = [&](mlir::Type t) {
+    t = fir::unwrapSequenceType(fir::dyn_cast_ptrEleTy(t));
+    return t.dyn_cast<fir::CharacterType>();
+  };
+  auto inTy = unwrap(getFrom().getType());
+  auto outTy = unwrap(getTo().getType());
+  if (!(inTy && outTy))
+    return emitOpError("not a reference to a character");
+  if (inTy.getFKind() == outTy.getFKind())
+    return emitOpError("buffers must have different KIND values");
+  return mlir::success();
+}
+
+//===----------------------------------------------------------------------===//
 // CmpOp
 //===----------------------------------------------------------------------===//
 
 template <typename OPTY>
 static void printCmpOp(mlir::OpAsmPrinter &p, OPTY op) {
   p << ' ';
   auto predSym = mlir::arith::symbolizeCmpFPredicate(
       op->template getAttrOfType<mlir::IntegerAttr>(
static mlir::ParseResult parseCmpOp(mlir::OpAsmParser &parser,
   attrs.set(OPTY::getPredicateAttrName(),
             builder.getI64IntegerAttr(static_cast<std::int64_t>(predicate)));
   result.attributes = attrs;
   result.addTypes({i1Type});
   return mlir::success();
 }
 
 //===----------------------------------------------------------------------===//
-// CharConvertOp
-//===----------------------------------------------------------------------===//
-
-mlir::LogicalResult fir::CharConvertOp::verify() {
-  auto unwrap = [&](mlir::Type t) {
-    t = fir::unwrapSequenceType(fir::dyn_cast_ptrEleTy(t));
-    return t.dyn_cast<fir::CharacterType>();
-  };
-  auto inTy = unwrap(getFrom().getType());
-  auto outTy = unwrap(getTo().getType());
-  if (!(inTy && outTy))
-    return emitOpError("not a reference to a character");
-  if (inTy.getFKind() == outTy.getFKind())
-    return emitOpError("buffers must have different KIND values");
-  return mlir::success();
-}
-
-//===----------------------------------------------------------------------===//
 // CmpcOp
 //===----------------------------------------------------------------------===//
 
 void fir::buildCmpCOp(mlir::OpBuilder &builder, mlir::OperationState &result,
                       mlir::arith::CmpFPredicate predicate, mlir::Value lhs,
                       mlir::Value rhs) {
   result.addOperands({lhs, rhs});
   result.types.push_back(builder.getI1Type());

flang/lib/Optimizer/Transforms/ArrayValueCopy.cpp

auto fromAddr = rewriter.create<ArrayCoorOp>(
       loc, getEleTy(src.getType()), src, shapeOp,
       CopyIn && copyUsingSlice ? sliceOp : mlir::Value{},
       factory::originateIndices(loc, rewriter, src.getType(), shapeOp, indices),
       getTypeParamsIfRawData(loc, builder, arrLoad, src.getType()));
   auto toAddr = rewriter.create<ArrayCoorOp>(
       loc, getEleTy(dst.getType()), dst, shapeOp,
       !CopyIn && copyUsingSlice ? sliceOp : mlir::Value{},
       factory::originateIndices(loc, rewriter, dst.getType(), shapeOp, indices),
-      getTypeParamsIfRawData(loc, builder, arrLoad, src.getType()));
+      getTypeParamsIfRawData(loc, builder, arrLoad, dst.getType()));
   auto eleTy = unwrapSequenceType(unwrapPassByRefType(dst.getType()));
   // Copy from (to) object to (from) temp copy of same object.
   if (auto charTy = eleTy.dyn_cast<CharacterType>()) {
     auto len = getCharacterLen(loc, builder, arrLoad, charTy);
     CharBoxValue toChar(toAddr, len);
     CharBoxValue fromChar(fromAddr, len);
     factory::genScalarAssignment(builder, loc, toChar, fromChar);
   } else {

flang/test/Lower/forall/character-1.f90

subroutine sub(x)
     forall (i=1:2)
        x(i:i)(1:1) = x(i+1:i+1)(1:1)
     end forall
     print *,x
   end subroutine sub
 end program test
 
 ! CHECK-LABEL: define void @_QFPsub(
-! CHECK-SAME: ptr %[[arg:.*]])
+! CHECK-SAME:    ptr %[[arg:.*]])
 ! CHECK: %[[extent:.*]] = getelementptr { {{.*}}, [1 x [3 x i64]] }, ptr %[[arg]], i32 0, i32 7, i64 0, i32 1
 ! CHECK: %[[extval:.*]] = load i64, ptr %[[extent]]
 ! CHECK: %[[elesize:.*]] = getelementptr { {{.*}}, [1 x [3 x i64]] }, ptr %[[arg]], i32 0, i32 1
 ! CHECK: %[[esval:.*]] = load i64, ptr %[[elesize]]
 ! CHECK: %[[mul:.*]] = mul i64 1, %[[esval]]
 ! CHECK: %[[mul2:.*]] = mul i64 %[[mul]], %[[extval]]
 ! CHECK: %[[buff:.*]] = call ptr @malloc(i64 %[[mul2]])
 ! CHECK: %[[to:.*]] = getelementptr i8, ptr %[[buff]], i64 %
 ! CHECK: call void @llvm.memmove.p0.p0.i64(ptr %[[to]], ptr %{{.*}}, i64 %{{.*}}, i1 false)
 ! CHECK: call void @free(ptr %[[buff]])
 ! CHECK: call ptr @_FortranAioBeginExternalListOutput