update codegen

flang/include/flang/Optimizer/Dialect/FIROps.h

@@ -51,6 +51,7 @@ struct DebuggingResource
 };
 
 class CoordinateIndicesAdaptor;
+using IntOrValue = llvm::PointerUnion<mlir::IntegerAttr, mlir::Value>;
 
 } // namespace fir
 
@@ -60,7 +61,6 @@ class CoordinateIndicesAdaptor;
 namespace fir {
 class CoordinateIndicesAdaptor {
 public:
-  using IntOrValue = llvm::PointerUnion<mlir::IntegerAttr, mlir::Value>;
   using value_type = IntOrValue;
 
   CoordinateIndicesAdaptor(mlir::DenseI32ArrayAttr fieldIndices,

flang/include/flang/Optimizer/Dialect/FIROps.td

@@ -1748,10 +1748,16 @@ def fir_CoordinateOp : fir_Op<"coordinate_of", [NoMemoryEffect]> {
     Unlike LLVM's GEP instruction, one cannot stride over the outermost
     reference; therefore, the leading 0 index must be omitted.
 
+    This operation can be used to index derived type fields, in which case
+    the operand is the name of the index field.
+
     ```
       %i = ... : index
       %h = ... : !fir.heap<!fir.array<100 x f32>>
       %p = fir.coordinate_of %h, %i : (!fir.heap<!fir.array<100 x f32>>, index) -> !fir.ref<f32>
+
+      %d = ... : !fir.ref<!fir.type<t{field1:i32, field2:f32}>>
+      %f = fir.coordinate_of %d, field2 : (!fir.ref<!fir.type<t{field1:i32, field2:f32}>>) -> !fir.ref<f32>
     ```
 
     In the example, `%p` will be a pointer to the `%i`-th f32 value in the
@@ -1772,7 +1778,9 @@ def fir_CoordinateOp : fir_Op<"coordinate_of", [NoMemoryEffect]> {
 
   let builders = [
     OpBuilder<(ins "mlir::Type":$resultType,
-      "mlir::Value":$ref, "mlir::ValueRange":$coor)>
+      "mlir::Value":$ref, "mlir::ValueRange":$coor)>,
+    OpBuilder<(ins "mlir::Type":$resultType,
+      "mlir::Value":$ref, "llvm::ArrayRef<fir::IntOrValue>":$coor)>
   ];
   let extraClassDeclaration = [{
     constexpr static int32_t kDynamicIndex = std::numeric_limits<int32_t>::min();

flang/lib/Lower/OpenMP/Utils.cpp

@@ -354,14 +354,12 @@ mlir::Value createParentSymAndGenIntermediateMaps(
     // type.
     if (fir::RecordType recordType = mlir::dyn_cast<fir::RecordType>(
             fir::unwrapPassByRefType(curValue.getType()))) {
-      mlir::Value idxConst = firOpBuilder.createIntegerConstant(
-          clauseLocation, firOpBuilder.getIndexType(),
-          indices[currentIndicesIdx]);
-      mlir::Type memberTy =
-          recordType.getTypeList().at(indices[currentIndicesIdx]).second;
+      fir::IntOrValue idxConst = mlir::IntegerAttr::get(
+          firOpBuilder.getI32Type(), indices[currentIndicesIdx]);
+      mlir::Type memberTy = recordType.getType(indices[currentIndicesIdx]);
       curValue = firOpBuilder.create<fir::CoordinateOp>(
           clauseLocation, firOpBuilder.getRefType(memberTy), curValue,
-          idxConst);
+          llvm::SmallVector<fir::IntOrValue, 1>{idxConst});
 
       // Skip mapping and the subsequent load if we're the final member or not
       // a type with a descriptor such as a pointer/allocatable. If we're a

flang/lib/Optimizer/CodeGen/CodeGen.cpp

@@ -2653,57 +2653,78 @@ struct CoordinateOpConversion
     return mlir::isa<fir::SequenceType, fir::RecordType, mlir::TupleType>(type);
   }
 
-  /// Check whether this form of `!fir.coordinate_of` is supported. These
-  /// additional checks are required, because we are not yet able to convert
-  /// all valid forms of `!fir.coordinate_of`.
-  /// TODO: Either implement the unsupported cases or extend the verifier
-  /// in FIROps.cpp instead.
-  static bool supportedCoordinate(mlir::Type type, mlir::ValueRange coors) {
-    const std::size_t numOfCoors = coors.size();
-    std::size_t i = 0;
-    bool subEle = false;
-    bool ptrEle = false;
-    for (; i < numOfCoors; ++i) {
-      mlir::Value nxtOpnd = coors[i];
-      if (auto arrTy = mlir::dyn_cast<fir::SequenceType>(type)) {
-        subEle = true;
-        i += arrTy.getDimension() - 1;
-        type = arrTy.getEleTy();
-      } else if (auto recTy = mlir::dyn_cast<fir::RecordType>(type)) {
-        subEle = true;
-        type = recTy.getType(getFieldNumber(recTy, nxtOpnd));
-      } else if (auto tupTy = mlir::dyn_cast<mlir::TupleType>(type)) {
-        subEle = true;
-        type = tupTy.getType(getConstantIntValue(nxtOpnd));
-      } else {
-        ptrEle = true;
-      }
-    }
-    if (ptrEle)
-      return (!subEle) && (numOfCoors == 1);
-    return subEle && (i >= numOfCoors);
-  }
+  // Helper structure to analyze the CoordinateOp path and decide if and how
+  // the GEP should be generated for it.
+  struct ShapeAnalysis {
+    bool hasKnownShape;
+    bool columnIsDeferred;
+  };
 
   /// Walk the abstract memory layout and determine if the path traverses any
   /// array types with unknown shape. Return true iff all the array types have a
   /// constant shape along the path.
-  static bool arraysHaveKnownShape(mlir::Type type, mlir::ValueRange coors) {
-    for (std::size_t i = 0, sz = coors.size(); i < sz; ++i) {
-      mlir::Value nxtOpnd = coors[i];
+  /// TODO: move the verification logic into the verifier.
+  static std::optional<ShapeAnalysis>
+  arraysHaveKnownShape(mlir::Type type, fir::CoordinateOp coor) {
+    fir::CoordinateIndicesAdaptor indices = coor.getIndices();
+    auto begin = indices.begin();
+    bool hasKnownShape = true;
+    bool columnIsDeferred = false;
+    for (auto it = begin, end = indices.end(); it != end;) {
       if (auto arrTy = mlir::dyn_cast<fir::SequenceType>(type)) {
-        if (fir::sequenceWithNonConstantShape(arrTy))
-          return false;
-        i += arrTy.getDimension() - 1;
+        bool addressingStart = (it == begin);
+        unsigned arrayDim = arrTy.getDimension();
+        for (auto dimExtent : llvm::enumerate(arrTy.getShape())) {
+          if (dimExtent.value() == fir::SequenceType::getUnknownExtent()) {
+            hasKnownShape = false;
+            if (addressingStart && dimExtent.index() + 1 == arrayDim) {
+              // If this point was reached, the raws of the first array have
+              // constant extents.
+              columnIsDeferred = true;
+            } else {
+              // One of the array dimension that is not the column of the first
+              // array has dynamic extent. It will not possible to do
+              // code generation for the CoordinateOp if the base is not a
+              // fir.box containing the value of that extent.
+              return ShapeAnalysis{false, false};
+            }
+          }
+          // There may be less operands than the array size if the
+          // fir.coordinate_of result is not an element but a sub-array.
+          if (it != end)
+            ++it;
+        }
         type = arrTy.getEleTy();
-      } else if (auto strTy = mlir::dyn_cast<fir::RecordType>(type)) {
-        type = strTy.getType(getFieldNumber(strTy, nxtOpnd));
+        continue;
+      }
+      if (auto strTy = mlir::dyn_cast<fir::RecordType>(type)) {
+        auto intAttr = llvm::dyn_cast<mlir::IntegerAttr>(*it);
+        if (!intAttr) {
+          mlir::emitError(coor.getLoc(),
+                          "expected field name in fir.coordinate_of");
+          return std::nullopt;
+        }
+        type = strTy.getType(intAttr.getInt());
       } else if (auto strTy = mlir::dyn_cast<mlir::TupleType>(type)) {
-        type = strTy.getType(getConstantIntValue(nxtOpnd));
-      } else {
-        return true;
+        auto value = llvm::dyn_cast<mlir::Value>(*it);
+        if (!value) {
+          mlir::emitError(
+              coor.getLoc(),
+              "expected constant value to address tuple in fir.coordinate_of");
+          return std::nullopt;
+        }
+        type = strTy.getType(getConstantIntValue(value));
+      } else if (auto charType = mlir::dyn_cast<fir::CharacterType>(type)) {
+        // Addressing character in string. Fortran strings degenerate to arrays
+        // in LLVM, so they are handled like arrays of characters here.
+        if (charType.getLen() == fir::CharacterType::unknownLen())
+          return ShapeAnalysis{false, true};
+        type = fir::CharacterType::getSingleton(charType.getContext(),
+                                                charType.getFKind());
       }
+      ++it;
     }
-    return true;
+    return ShapeAnalysis{hasKnownShape, columnIsDeferred};
   }
 
 private:
@@ -2754,36 +2775,43 @@ struct CoordinateOpConversion
     mlir::LLVM::IntegerOverflowFlags nsw =
         mlir::LLVM::IntegerOverflowFlags::nsw;
 
-    for (unsigned i = 1, last = operands.size(); i < last; ++i) {
+    int nextIndexValue = 1;
+    fir::CoordinateIndicesAdaptor indices = coor.getIndices();
+    for (auto it = indices.begin(), end = indices.end(); it != end;) {
       if (auto arrTy = mlir::dyn_cast<fir::SequenceType>(cpnTy)) {
-        if (i != 1)
+        if (it != indices.begin())
           TODO(loc, "fir.array nested inside other array and/or derived type");
         // Applies byte strides from the box. Ignore lower bound from box
         // since fir.coordinate_of indexes are zero based. Lowering takes care
         // of lower bound aspects. This both accounts for dynamically sized
         // types and non contiguous arrays.
         auto idxTy = lowerTy().indexType();
         mlir::Value off = genConstantIndex(loc, idxTy, rewriter, 0);
-        for (unsigned index = i, lastIndex = i + arrTy.getDimension();
-             index < lastIndex; ++index) {
-          mlir::Value stride = getStrideFromBox(loc, boxTyPair, operands[0],
-                                                index - i, rewriter);
+        unsigned arrayDim = arrTy.getDimension();
+        for (unsigned dim = 0; dim < arrayDim && it != end; ++dim, ++it) {
+          mlir::Value stride =
+              getStrideFromBox(loc, boxTyPair, operands[0], dim, rewriter);
           auto sc = rewriter.create<mlir::LLVM::MulOp>(
-              loc, idxTy, operands[index], stride, nsw);
+              loc, idxTy, operands[nextIndexValue + dim], stride, nsw);
           off = rewriter.create<mlir::LLVM::AddOp>(loc, idxTy, sc, off, nsw);
         }
+        nextIndexValue += arrayDim;
         resultAddr = rewriter.create<mlir::LLVM::GEPOp>(
             loc, llvmPtrTy, byteTy, resultAddr,
             llvm::ArrayRef<mlir::LLVM::GEPArg>{off});
-        i += arrTy.getDimension() - 1;
         cpnTy = arrTy.getEleTy();
       } else if (auto recTy = mlir::dyn_cast<fir::RecordType>(cpnTy)) {
-        mlir::Value nxtOpnd = operands[i];
-        cpnTy = recTy.getType(getFieldNumber(recTy, nxtOpnd));
+        auto intAttr = llvm::dyn_cast<mlir::IntegerAttr>(*it);
+        if (!intAttr)
+          return mlir::emitError(loc,
+                                 "expected field name in fir.coordinate_of");
+        int fieldIndex = intAttr.getInt();
+        ++it;
+        cpnTy = recTy.getType(fieldIndex);
         auto llvmRecTy = lowerTy().convertType(recTy);
         resultAddr = rewriter.create<mlir::LLVM::GEPOp>(
             loc, llvmPtrTy, llvmRecTy, resultAddr,
-            llvm::ArrayRef<mlir::LLVM::GEPArg>{0, nxtOpnd});
+            llvm::ArrayRef<mlir::LLVM::GEPArg>{0, fieldIndex});
       } else {
         fir::emitFatalError(loc, "unexpected type in coordinate_of");
       }
@@ -2801,92 +2829,71 @@ struct CoordinateOpConversion
 
     // Component Type
     mlir::Type cpnTy = fir::dyn_cast_ptrOrBoxEleTy(baseObjectTy);
-    bool hasSubdimension = hasSubDimensions(cpnTy);
-    bool columnIsDeferred = !hasSubdimension;
-
-    if (!supportedCoordinate(cpnTy, operands.drop_front(1)))
-      TODO(loc, "unsupported combination of coordinate operands");
-
-    const bool hasKnownShape =
-        arraysHaveKnownShape(cpnTy, operands.drop_front(1));
-
-    // If only the column is `?`, then we can simply place the column value in
-    // the 0-th GEP position.
-    if (auto arrTy = mlir::dyn_cast<fir::SequenceType>(cpnTy)) {
-      if (!hasKnownShape) {
-        const unsigned sz = arrTy.getDimension();
-        if (arraysHaveKnownShape(arrTy.getEleTy(),
-                                 operands.drop_front(1 + sz))) {
-          fir::SequenceType::ShapeRef shape = arrTy.getShape();
-          bool allConst = true;
-          for (unsigned i = 0; i < sz - 1; ++i) {
-            if (shape[i] < 0) {
-              allConst = false;
-              break;
-            }
-          }
-          if (allConst)
-            columnIsDeferred = true;
-        }
-      }
-    }
+
+    const std::optional<ShapeAnalysis> shapeAnalysis =
+        arraysHaveKnownShape(cpnTy, coor);
+    if (!shapeAnalysis)
+      return mlir::failure();
 
     if (fir::hasDynamicSize(fir::unwrapSequenceType(cpnTy)))
       return mlir::emitError(
           loc, "fir.coordinate_of with a dynamic element size is unsupported");
 
-    if (hasKnownShape || columnIsDeferred) {
+    if (shapeAnalysis->hasKnownShape || shapeAnalysis->columnIsDeferred) {
       llvm::SmallVector<mlir::LLVM::GEPArg> offs;
-      if (hasKnownShape && hasSubdimension) {
+      if (shapeAnalysis->hasKnownShape) {
         offs.push_back(0);
       }
+      // Else, only the column is `?` and we can simply place the column value
+      // in the 0-th GEP position.
+
       std::optional<int> dims;
       llvm::SmallVector<mlir::Value> arrIdx;
-      for (std::size_t i = 1, sz = operands.size(); i < sz; ++i) {
-        mlir::Value nxtOpnd = operands[i];
-
-        if (!cpnTy)
-          return mlir::emitError(loc, "invalid coordinate/check failed");
-
-        // check if the i-th coordinate relates to an array
-        if (dims) {
-          arrIdx.push_back(nxtOpnd);
-          int dimsLeft = *dims;
-          if (dimsLeft > 1) {
-            dims = dimsLeft - 1;
-            continue;
-          }
-          cpnTy = mlir::cast<fir::SequenceType>(cpnTy).getElementType();
-          // append array range in reverse (FIR arrays are column-major)
-          offs.append(arrIdx.rbegin(), arrIdx.rend());
-          arrIdx.clear();
-          dims.reset();
+      int nextIndexValue = 1;
+      for (auto index : coor.getIndices()) {
+        if (auto intAttr = llvm::dyn_cast<mlir::IntegerAttr>(index)) {
+          // Addressing derived type component.
+          auto recordType = llvm::dyn_cast<fir::RecordType>(cpnTy);
+          if (!recordType)
+            return mlir::emitError(
+                loc,
+                "fir.coordinate base type is not consistent with operands");
+          int fieldId = intAttr.getInt();
+          cpnTy = recordType.getType(fieldId);
+          offs.push_back(fieldId);
           continue;
         }
-        if (auto arrTy = mlir::dyn_cast<fir::SequenceType>(cpnTy)) {
-          int d = arrTy.getDimension() - 1;
-          if (d > 0) {
-            dims = d;
-            arrIdx.push_back(nxtOpnd);
-            continue;
+        // Value index (addressing array, tuple, or complex part).
+        mlir::Value indexValue = operands[nextIndexValue++];
+        if (auto tupTy = mlir::dyn_cast<mlir::TupleType>(cpnTy)) {
+          cpnTy = tupTy.getType(getConstantIntValue(indexValue));
+          offs.push_back(indexValue);
+        } else {
+          if (!dims) {
+            if (auto arrayType = llvm::dyn_cast<fir::SequenceType>(cpnTy)) {
+              // Starting addressing array or array component.
+              dims = arrayType.getDimension();
+              cpnTy = arrayType.getElementType();
+            }
+          }
+          if (dims) {
+            arrIdx.push_back(indexValue);
+            if (--(*dims) == 0) {
+              // Append array range in reverse (FIR arrays are column-major).
+              offs.append(arrIdx.rbegin(), arrIdx.rend());
+              arrIdx.clear();
+              dims.reset();
+            }
+          } else {
+            offs.push_back(indexValue);
           }
-          cpnTy = mlir::cast<fir::SequenceType>(cpnTy).getElementType();
-          offs.push_back(nxtOpnd);
-          continue;
         }
-
-        // check if the i-th coordinate relates to a field
-        if (auto recTy = mlir::dyn_cast<fir::RecordType>(cpnTy))
-          cpnTy = recTy.getType(getFieldNumber(recTy, nxtOpnd));
-        else if (auto tupTy = mlir::dyn_cast<mlir::TupleType>(cpnTy))
-          cpnTy = tupTy.getType(getConstantIntValue(nxtOpnd));
-        else
-          cpnTy = nullptr;
-
-        offs.push_back(nxtOpnd);
       }
-      if (dims)
+      // It is possible the fir.coordinate_of result is a sub-array, in which
+      // case there may be some "unfinished" array indices to reverse and push.
+      if (!arrIdx.empty())
         offs.append(arrIdx.rbegin(), arrIdx.rend());
+
       mlir::Value base = operands[0];
       mlir::Value retval = genGEP(loc, llvmObjectTy, rewriter, base, offs);
       rewriter.replaceOp(coor, retval);