[flang] Update the conversion code for fir.coordinate_of

[awarzynski]

Summary

These are mostly small changes to make the code a bit clearer and more
consistent. Summary of changes:

• add missing namespace qualifiers (that's the preference in Flang)
• replace const member methods with static methods (to avoid passing the *this pointer unnecessarily)
• rename currentObjTy (current object type) as cpnTy (component type) - the latter feels more fitting
• remove redundant return failure(); calls ( return mlir::emitError gives the same result)
• updated a few comments

[clementval]

Can you fix the clang-format problems?

Is this an update from changes made while syncing back to fir-dev?

[awarzynski]

Fix formatting

[awarzynski]

In D122799#3421480, @clementval wrote:

Can you fix the clang-format problems?

Sorry about that. I've tried debugging Arcanist, but to no avail. Annoyingly, it does work occasionally :/

Is this an update from changes made while syncing back to fir-dev?

Yes.

[ekieri]

flang/lib/Optimizer/CodeGen/CodeGen.cpp

2304–2306

nit: You could limit the scope of sz by initialising it in the init slot of the loop together with i (as you do further down). Won't make much difference in this case, but for the good habit.

[kiranchandramohan]

LGTM.

[awarzynski]

flang/lib/Optimizer/CodeGen/CodeGen.cpp

2304–2306

Good point, thanks!

[awarzynski]

Move the definition of sz

[schweitz]

Thanks for upstreaming these, Andrzej.

flang/lib/Optimizer/CodeGen/CodeGen.cpp

doRewrite(fir::CoordinateOp coor, mlir::Type ty, OpAdaptor adaptor,
     mlir::Type baseObjectTy = coor.getBaseType();
     mlir::Type objectTy = fir::dyn_cast_ptrOrBoxEleTy(baseObjectTy);
     assert(objectTy && "fir.coordinate_of expects a reference type");
 
     // Complex type - basically, extract the real or imaginary part
     if (fir::isa_complex(objectTy)) {
       mlir::LLVM::ConstantOp c0 =
           genConstantIndex(loc, lowerTy().indexType(), rewriter, 0);
-      SmallVector<mlir::Value> offs = {c0, operands[1]};
+      llvm::SmallVector<mlir::Value> offs = {c0, operands[1]};
       mlir::Value gep = genGEP(loc, ty, rewriter, base, offs);
       rewriter.replaceOp(coor, gep);
       return success();
     }
 
     // Boxed type - get the base pointer from the box
     if (baseObjectTy.dyn_cast<fir::BoxType>())
       return doRewriteBox(coor, ty, operands, loc, rewriter);
 
-    // Reference or pointer type
-    if (baseObjectTy.isa<fir::ReferenceType, fir::PointerType>())
+    // Reference, pointer or a heap type
+    if (baseObjectTy.isa<fir::ReferenceType, fir::PointerType, fir::HeapType>())
       return doRewriteRefOrPtr(coor, ty, operands, loc, rewriter);
 
     return rewriter.notifyMatchFailure(
         coor, "fir.coordinate_of base operand has unsupported type");
   }
 
-  unsigned getFieldNumber(fir::RecordType ty, mlir::Value op) const {
+  static unsigned getFieldNumber(fir::RecordType ty, mlir::Value op) {
     return fir::hasDynamicSize(ty)
                ? op.getDefiningOp()
                      ->getAttrOfType<mlir::IntegerAttr>("field")
                      .getInt()
                : getIntValue(op);
   }
 
-  int64_t getIntValue(mlir::Value val) const {
+  static int64_t getIntValue(mlir::Value val) {
     assert(val && val.dyn_cast<mlir::OpResult>() && "must not be null value");
     mlir::Operation *defop = val.getDefiningOp();
 
     if (auto constOp = dyn_cast<mlir::arith::ConstantIntOp>(defop))
       return constOp.value();
     if (auto llConstOp = dyn_cast<mlir::LLVM::ConstantOp>(defop))
       if (auto attr = llConstOp.getValue().dyn_cast<mlir::IntegerAttr>())
         return attr.getValue().getSExtValue();
     fir::emitFatalError(val.getLoc(), "must be a constant");
   }
 
-  bool hasSubDimensions(mlir::Type type) const {
+  static bool hasSubDimensions(mlir::Type type) {
     return type.isa<fir::SequenceType, fir::RecordType, mlir::TupleType>();
   }
 
   /// 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.
-  bool supportedCoordinate(mlir::Type type, mlir::ValueRange coors) const {
+  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 = type.dyn_cast<fir::SequenceType>()) {
         subEle = true;
static bool supportedCoordinate(mlir::Type type, mlir::ValueRange coors) {
     }
     if (ptrEle)
       return (!subEle) && (numOfCoors == 1);
     return subEle && (i >= numOfCoors);
   }
 
   /// 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.
-  bool arraysHaveKnownShape(mlir::Type type, mlir::ValueRange coors) const {
-    const std::size_t sz = coors.size();
-    std::size_t i = 0;
-    for (; i < sz; ++i) {
+  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];
       if (auto arrTy = type.dyn_cast<fir::SequenceType>()) {
         if (fir::sequenceWithNonConstantShape(arrTy))
           return false;
         i += arrTy.getDimension() - 1;
         type = arrTy.getEleTy();
       } else if (auto strTy = type.dyn_cast<fir::RecordType>()) {
         type = strTy.getType(getFieldNumber(strTy, nxtOpnd));
doRewriteBox(fir::CoordinateOp coor, mlir::Type ty, mlir::ValueRange operands,
 
     // 1. SPECIAL CASE (uses `fir.len_param_index`):
     //   %box = ... : !fir.box<!fir.type<derived{len1:i32}>>
     //   %lenp = fir.len_param_index len1, !fir.type<derived{len1:i32}>
     //   %addr = coordinate_of %box, %lenp
     if (coor.getNumOperands() == 2) {
       mlir::Operation *coordinateDef =
           (*coor.getCoor().begin()).getDefiningOp();
-      if (isa_and_nonnull<fir::LenParamIndexOp>(coordinateDef)) {
+      if (isa_and_nonnull<fir::LenParamIndexOp>(coordinateDef))
         TODO(loc,
              "fir.coordinate_of - fir.len_param_index is not supported yet");
-      }
-    }
 
     // 2. GENERAL CASE:
     // 2.1. (`fir.array`)
     //   %box = ... : !fix.box<!fir.array<?xU>>
     //   %idx = ... : index
     //   %resultAddr = coordinate_of %box, %idx : !fir.ref<U>
     // 2.2 (`fir.derived`)
     //   %box = ... : !fix.box<!fir.type<derived_type{field_1:i32}>>
     //   %idx = ... : i32
     //   %resultAddr = coordinate_of %box, %idx : !fir.ref<i32>
     // 2.3 (`fir.derived` inside `fir.array`)
     //   %box = ... : !fir.box<!fir.array<10 x !fir.type<derived_1{field_1:f32,
     //   field_2:f32}>>> %idx1 = ... : index %idx2 = ... : i32 %resultAddr =
     //   coordinate_of %box, %idx1, %idx2 : !fir.ref<f32>
     // 2.4. TODO: Either document or disable any other case that the following
     //  implementation might convert.
     mlir::LLVM::ConstantOp c0 =
         genConstantIndex(loc, lowerTy().indexType(), rewriter, 0);
     mlir::Value resultAddr =
         loadBaseAddrFromBox(loc, getBaseAddrTypeFromBox(boxBaseAddr.getType()),
                             boxBaseAddr, rewriter);
-    auto currentObjTy = fir::dyn_cast_ptrOrBoxEleTy(boxObjTy);
+    // Component Type
+    auto cpnTy = fir::dyn_cast_ptrOrBoxEleTy(boxObjTy);
     mlir::Type voidPtrTy = ::getVoidPtrType(coor.getContext());
 
     for (unsigned i = 1, last = operands.size(); i < last; ++i) {
-      if (auto arrTy = currentObjTy.dyn_cast<fir::SequenceType>()) {
+      if (auto arrTy = cpnTy.dyn_cast<fir::SequenceType>()) {
         if (i != 1)
           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 =
               loadStrideFromBox(loc, operands[0], index - i, rewriter);
           auto sc = rewriter.create<mlir::LLVM::MulOp>(loc, idxTy,
                                                        operands[index], stride);
           off = rewriter.create<mlir::LLVM::AddOp>(loc, idxTy, sc, off);
         }
         auto voidPtrBase =
             rewriter.create<mlir::LLVM::BitcastOp>(loc, voidPtrTy, resultAddr);
         SmallVector<mlir::Value> args{off};
         resultAddr = rewriter.create<mlir::LLVM::GEPOp>(loc, voidPtrTy,
                                                         voidPtrBase, args);
         i += arrTy.getDimension() - 1;
-        currentObjTy = arrTy.getEleTy();
-      } else if (auto recTy = currentObjTy.dyn_cast<fir::RecordType>()) {
+        cpnTy = arrTy.getEleTy();
+      } else if (auto recTy = cpnTy.dyn_cast<fir::RecordType>()) {
         auto recRefTy =
             mlir::LLVM::LLVMPointerType::get(lowerTy().convertType(recTy));
         mlir::Value nxtOpnd = operands[i];
         auto memObj =
             rewriter.create<mlir::LLVM::BitcastOp>(loc, recRefTy, resultAddr);
         llvm::SmallVector<mlir::Value> args = {c0, nxtOpnd};
-        currentObjTy = recTy.getType(getFieldNumber(recTy, nxtOpnd));
-        auto llvmCurrentObjTy = lowerTy().convertType(currentObjTy);
+        cpnTy = recTy.getType(getFieldNumber(recTy, nxtOpnd));
+        auto llvmCurrentObjTy = lowerTy().convertType(cpnTy);
         auto gep = rewriter.create<mlir::LLVM::GEPOp>(
             loc, mlir::LLVM::LLVMPointerType::get(llvmCurrentObjTy), memObj,
             args);
         resultAddr =
             rewriter.create<mlir::LLVM::BitcastOp>(loc, voidPtrTy, gep);
       } else {
         fir::emitFatalError(loc, "unexpected type in coordinate_of");
       }
     }
 
     rewriter.replaceOpWithNewOp<mlir::LLVM::BitcastOp>(coor, ty, resultAddr);
     return success();
   }
 
   mlir::LogicalResult
   doRewriteRefOrPtr(fir::CoordinateOp coor, mlir::Type ty,
                     mlir::ValueRange operands, mlir::Location loc,
                     mlir::ConversionPatternRewriter &rewriter) const {
     mlir::Type baseObjectTy = coor.getBaseType();
 
-    mlir::Type currentObjTy = fir::dyn_cast_ptrOrBoxEleTy(baseObjectTy);
-    bool hasSubdimension = hasSubDimensions(currentObjTy);
+    // Component Type
+    mlir::Type cpnTy = fir::dyn_cast_ptrOrBoxEleTy(baseObjectTy);
+    bool hasSubdimension = hasSubDimensions(cpnTy);
     bool columnIsDeferred = !hasSubdimension;
 
-    if (!supportedCoordinate(currentObjTy, operands.drop_front(1))) {
+    if (!supportedCoordinate(cpnTy, operands.drop_front(1)))
       TODO(loc, "unsupported combination of coordinate operands");
-    }
 
     const bool hasKnownShape =
-        arraysHaveKnownShape(currentObjTy, operands.drop_front(1));
+        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 = currentObjTy.dyn_cast<fir::SequenceType>()) {
+    if (auto arrTy = cpnTy.dyn_cast<fir::SequenceType>()) {
       if (!hasKnownShape) {
         const unsigned sz = arrTy.getDimension();
         if (arraysHaveKnownShape(arrTy.getEleTy(),
                                  operands.drop_front(1 + sz))) {
-          llvm::ArrayRef<int64_t> shape = arrTy.getShape();
+          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;
         }
       }
     }
 
-    if (fir::hasDynamicSize(fir::unwrapSequenceType(currentObjTy))) {
-      mlir::emitError(
+    if (fir::hasDynamicSize(fir::unwrapSequenceType(cpnTy)))
+      return mlir::emitError(
           loc, "fir.coordinate_of with a dynamic element size is unsupported");
-      return failure();
-    }
 
     if (hasKnownShape || columnIsDeferred) {
       SmallVector<mlir::Value> offs;
       if (hasKnownShape && hasSubdimension) {
         mlir::LLVM::ConstantOp c0 =
             genConstantIndex(loc, lowerTy().indexType(), rewriter, 0);
         offs.push_back(c0);
       }
-      const std::size_t sz = operands.size();
       Optional<int> dims;
       SmallVector<mlir::Value> arrIdx;
-      for (std::size_t i = 1; i < sz; ++i) {
+      for (std::size_t i = 1, sz = operands.size(); i < sz; ++i) {
         mlir::Value nxtOpnd = operands[i];
 
-        if (!currentObjTy) {
-          mlir::emitError(loc, "invalid coordinate/check failed");
-          return failure();
-        }
+        if (!cpnTy)
+          return mlir::emitError(loc, "invalid coordinate/check failed");
 
         // check if the i-th coordinate relates to an array
         if (dims.hasValue()) {
           arrIdx.push_back(nxtOpnd);
           int dimsLeft = *dims;
           if (dimsLeft > 1) {
             dims = dimsLeft - 1;
             continue;
           }
-          currentObjTy = currentObjTy.cast<fir::SequenceType>().getEleTy();
+          cpnTy = cpnTy.cast<fir::SequenceType>().getEleTy();
           // append array range in reverse (FIR arrays are column-major)
           offs.append(arrIdx.rbegin(), arrIdx.rend());
           arrIdx.clear();
           dims.reset();
           continue;
         }
-        if (auto arrTy = currentObjTy.dyn_cast<fir::SequenceType>()) {
+        if (auto arrTy = cpnTy.dyn_cast<fir::SequenceType>()) {
           int d = arrTy.getDimension() - 1;
           if (d > 0) {
             dims = d;
             arrIdx.push_back(nxtOpnd);
             continue;
           }
-          currentObjTy = currentObjTy.cast<fir::SequenceType>().getEleTy();
+          cpnTy = cpnTy.cast<fir::SequenceType>().getEleTy();
           offs.push_back(nxtOpnd);
           continue;
         }
 
         // check if the i-th coordinate relates to a field
-        if (auto recTy = currentObjTy.dyn_cast<fir::RecordType>())
-          currentObjTy = recTy.getType(getFieldNumber(recTy, nxtOpnd));
-        else if (auto tupTy = currentObjTy.dyn_cast<mlir::TupleType>())
-          currentObjTy = tupTy.getType(getIntValue(nxtOpnd));
+        if (auto recTy = cpnTy.dyn_cast<fir::RecordType>())
+          cpnTy = recTy.getType(getFieldNumber(recTy, nxtOpnd));
+        else if (auto tupTy = cpnTy.dyn_cast<mlir::TupleType>())
+          cpnTy = tupTy.getType(getIntValue(nxtOpnd));
         else
-          currentObjTy = nullptr;
+          cpnTy = nullptr;
 
         offs.push_back(nxtOpnd);
       }
       if (dims.hasValue())
         offs.append(arrIdx.rbegin(), arrIdx.rend());
       mlir::Value base = operands[0];
       mlir::Value retval = genGEP(loc, ty, rewriter, base, offs);
       rewriter.replaceOp(coor, retval);
       return success();
     }
 
-    mlir::emitError(loc, "fir.coordinate_of base operand has unsupported type");
-    return failure();
+    return mlir::emitError(
+        loc, "fir.coordinate_of base operand has unsupported type");
   }
 };
 
 /// Convert `fir.field_index`. The conversion depends on whether the size of
 /// the record is static or dynamic.
 struct FieldIndexOpConversion : public FIROpConversion<fir::FieldIndexOp> {
   using FIROpConversion::FIROpConversion;