Remove builder that takes SSA value instead of Attribute on ExtractValueOp, InsetValueOp, and InsertOnRangeOp

[mehdi_amini]

Summary

This builder exposed a somehow "unsafe" API: it pretends we can
construct an InsertOnRangeOp from a range of SSA values, even though
this will crash if these aren't the result of arith.constant since
the operation actually needs attribute values (a build method can't
fail gracefully).
That means that the caller must check for the producer, at which
point they can just assemble the attribute array directly and call
the existing builder.

The existing call-sites were even in a worse state here: they would
actually create a constant operation that wouldn't be used and only
serve to carry the attribute through the builder API.

[clementval]

The pre-merge checks are failing during the build. Looks like there is a use of the builder somewhere.

[mehdi_amini]

In D112946#3101033, @clementval wrote:

The pre-merge checks are failing during the build. Looks like there is a use of the builder somewhere.

Indeed it was passing before I removed the declaration, which I think shows that this is dead code, but transitively speaking!

[clementval]

In D112946#3101041, @mehdi_amini wrote:

In D112946#3101033, @clementval wrote:

The pre-merge checks are failing during the build. Looks like there is a use of the builder somewhere.

Indeed it was passing before I removed the declaration, which I think shows that this is dead code, but transitively speaking!

This is probably left over from the version of the op that had values instead of attributes.

[mehdi_amini]

Fix patch: I had removed the declaration on the wrong op!

[mehdi_amini]

Turns out I had removed the declaration on InsertValueOp instead of InsertValueOp (/facepalm...)

[mehdi_amini]

(that said it showed me some issued with this API on InsertValueOp, I'll follow up.

[clementval]

In D112946#3101103, @mehdi_amini wrote:

(that said it showed me some issued with this API on InsertValueOp, I'll follow up.

I guess they have not been cleaned after moving to attribute as well.

[mehdi_amini]

Update to handle ExtractValueOp, InsetValueOp, and InsertOnRangeOp

[mehdi_amini]

flang/lib/Optimizer/Builder/Character.cpp

697

All these users were emitting spurious operations here!

[clementval]

This looks ok to me. This will need some work downstream since the old build functions is used in couple of places with fir::FieldIndexOp and it is handy to extract the two coordinates attributes from the op and append them as attribute. But I guess we can refactor that.

[schweitz]

In D112946#3101053, @clementval wrote:

In D112946#3101041, @mehdi_amini wrote:

In D112946#3101033, @clementval wrote:

The pre-merge checks are failing during the build. Looks like there is a use of the builder somewhere.

Indeed it was passing before I removed the declaration, which I think shows that this is dead code, but transitively speaking!

This is probably left over from the version of the op that had values instead of attributes.

The code in the lowering directories is very out of date.

[mehdi_amini]

In D112946#3102451, @clementval wrote:

This looks ok to me.

Great, I assume I can land this then!

[clementval]

In D112946#3104478, @mehdi_amini wrote:

In D112946#3102451, @clementval wrote:

This looks ok to me.

Great, I assume I can land this then!

I was still checking the repercussion of this on the code that still need to be upstreamed. Next time please wait for the formal approval.

[mehdi_amini]

In D112946#3105442, @clementval wrote:

In D112946#3104478, @mehdi_amini wrote:

In D112946#3102451, @clementval wrote:

This looks ok to me.

Great, I assume I can land this then!

I was still checking the repercussion of this on the code that still need to be upstreamed. Next time please wait for the formal approval.

Ah sorry about that! Feel free to revert if there are deeper issues with the direction.

In general I was expecting you to keep this kind of code in your fork as needed to ease the update.

flang/include/flang/Lower/ComplexExpr.h

public:
   }
 
   mlir::Value createComplexCompare(mlir::Value cplx1, mlir::Value cplx2,
                                    bool eq);
 
 protected:
   template <Part partId>
   mlir::Value extract(mlir::Value cplx) {
-    return builder.create<fir::ExtractValueOp>(loc, getComplexPartType(cplx),
-                                               cplx, createPartId<partId>());
+    return builder.create<fir::ExtractValueOp>(
+        loc, getComplexPartType(cplx), cplx,
+        builder.getArrayAttr({builder.getIntegerAttr(
+            builder.getIndexType(), static_cast<int>(partId))}));
   }
 
   template <Part partId>
   mlir::Value insert(mlir::Value cplx, mlir::Value part) {
-    return builder.create<fir::InsertValueOp>(loc, cplx.getType(), cplx, part,
-                                              createPartId<partId>());
+    return builder.create<fir::InsertValueOp>(
+        loc, cplx.getType(), cplx, part,
+        builder.getArrayAttr({builder.getIntegerAttr(
+            builder.getIndexType(), static_cast<int>(partId))}));
   }
 
   template <Part partId>
   mlir::Value createPartId() {
     return builder.createIntegerConstant(loc, builder.getIndexType(),
                                          static_cast<int>(partId));
   }
 
 private:
   FirOpBuilder &builder;
   mlir::Location loc;
 };
 
 } // namespace Fortran::lower
 
 #endif // FORTRAN_LOWER_COMPLEXEXPR_H

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

def fir_ExtractValueOp : fir_OneResultOp<"extract_value", [NoSideEffect]> {
   let arguments = (ins
     AnyCompositeLike:$adt,
     ArrayAttr:$coor
   );
 
   let assemblyFormat = [{
     $adt `,` $coor attr-dict `:` functional-type(operands, results)
   }];
-
-  let builders = [
-    OpBuilder<(ins "mlir::Type":$rty, "mlir::Value":$adt,
-      "llvm::ArrayRef<mlir::Value>":$vcoor)>
-  ];
 }
 
 def fir_FieldIndexOp : fir_OneResultOp<"field_index", [NoSideEffect]> {
   let summary = "create a field index value from a field identifier";
 
   let description = [{
     Generate a field (offset) value from an identifier.  Field values may be
     lowered into exact offsets when the layout of a Fortran derived type is
def fir_InsertValueOp : fir_OneResultOp<"insert_value", [NoSideEffect]> {
 
   let arguments = (ins AnyCompositeLike:$adt, AnyType:$val, ArrayAttr:$coor);
   let results = (outs AnyCompositeLike);
 
   let assemblyFormat = [{
     $adt `,` $val `,` $coor attr-dict `:` functional-type(operands, results)
   }];
 
-  let builders = [
-    OpBuilder<(ins "mlir::Type":$rty, "mlir::Value":$adt, "mlir::Value":$val,
-      "llvm::ArrayRef<mlir::Value>":$vcoor)>
-  ];
-
   let hasCanonicalizer = 1;
 }
 
 def fir_InsertOnRangeOp : fir_OneResultOp<"insert_on_range", [NoSideEffect]> {
   let summary = "insert sub-value into a range on an existing sequence";
 
   let description = [{
     Insert copies of a value into an entity with an array type.
def fir_InsertOnRangeOp : fir_OneResultOp<"insert_on_range", [NoSideEffect]> {
 
   let arguments = (ins fir_SequenceType:$seq, AnyType:$val, ArrayAttr:$coor);
   let results = (outs fir_SequenceType);
 
   let assemblyFormat = [{
     $seq `,` $val `,` $coor attr-dict `:` functional-type(operands, results)
   }];
 
-  let builders = [
-    OpBuilder<(ins "mlir::Type":$rty, "mlir::Value":$adt, "mlir::Value":$val,
-      "llvm::ArrayRef<mlir::Value>":$vcoor)>
-  ];
-
   let verifier = "return ::verify(*this);";
 }
 
 def fir_LenParamIndexOp : fir_OneResultOp<"len_param_index", [NoSideEffect]> {
   let summary =
     "create a field index value from a LEN type parameter identifier";
 
   let description = [{

flang/lib/Lower/CharacterExpr.cpp

void Fortran::lower::CharacterExprHelper::createLengthOneAssign(
   auto addr = lhs.getBuffer();
   auto val = rhs.getBuffer();
   // If rhs value resides in memory, load it.
   if (!needToMaterialize(rhs))
     val = builder.create<fir::LoadOp>(loc, val);
   auto valTy = val.getType();
   // Precondition is rhs is size 1, but it may be wrapped in a fir.array.
   if (auto seqTy = valTy.dyn_cast<fir::SequenceType>()) {
-    auto zero = builder.createIntegerConstant(loc, builder.getIndexType(), 0);
+    auto zero = builder.getIntegerAttr(builder.getIndexType(), 0);
     valTy = seqTy.getEleTy();
-    val = builder.create<fir::ExtractValueOp>(loc, valTy, val, zero);
+    val = builder.create<fir::ExtractValueOp>(loc, valTy, val,
+                                              builder.getArrayAttr(zero));
   }
   auto addrTy = fir::ReferenceType::get(valTy);
   addr = builder.createConvert(loc, addrTy, addr);
   assert(fir::dyn_cast_ptrEleTy(addr.getType()) == val.getType());
   builder.create<fir::StoreOp>(loc, val, addr);
 }
 
 void Fortran::lower::CharacterExprHelper::createAssign(

flang/lib/Optimizer/Builder/Character.cpp

 mlir::Value
 fir::factory::CharacterExprHelper::createSingletonFromCode(mlir::Value code,
                                                            int kind) {
   auto charType = fir::CharacterType::get(builder.getContext(), kind, 1);
   auto bits = builder.getKindMap().getCharacterBitsize(kind);
   auto intType = builder.getIntegerType(bits);
   auto cast = builder.createConvert(loc, intType, code);
   auto undef = builder.create<fir::UndefOp>(loc, charType);
-  auto zero = builder.createIntegerConstant(loc, builder.getIndexType(), 0);
-  return builder.create<fir::InsertValueOp>(loc, charType, undef, cast, zero);
+  auto zero = builder.getIntegerAttr(builder.getIndexType(), 0);
+  return builder.create<fir::InsertValueOp>(loc, charType, undef, cast,
+                                            builder.getArrayAttr(zero));
 }
 
 mlir::Value fir::factory::CharacterExprHelper::extractCodeFromSingleton(
     mlir::Value singleton) {
   auto type = getCharacterType(singleton);
   assert(type.getLen() == 1);
   auto bits = builder.getKindMap().getCharacterBitsize(type.getFKind());
   auto intType = builder.getIntegerType(bits);
-  auto zero = builder.createIntegerConstant(loc, builder.getIndexType(), 0);
-  return builder.create<fir::ExtractValueOp>(loc, intType, singleton, zero);
+  auto zero = builder.getIntegerAttr(builder.getIndexType(), 0);
+  return builder.create<fir::ExtractValueOp>(loc, intType, singleton,
+                                             builder.getArrayAttr(zero));
 }
 
 mlir::Value
 fir::factory::CharacterExprHelper::readLengthFromBox(mlir::Value box) {
   auto lenTy = builder.getCharacterLengthType();
   auto size = builder.create<fir::BoxEleSizeOp>(loc, lenTy, box);
   auto charTy = recoverCharacterType(box.getType());
   auto bits = builder.getKindMap().getCharacterBitsize(charTy.getFKind());
   auto width = bits / 8;

flang/lib/Optimizer/Dialect/FIROps.cpp

 }
 
 mlir::ParseResult fir::GlobalOp::verifyValidLinkage(StringRef linkage) {
   // Supporting only a subset of the LLVM linkage types for now
   static const char *validNames[] = {"common", "internal", "linkonce", "weak"};
   return mlir::success(llvm::is_contained(validNames, linkage));
 }
 
-template <bool AllowFields>
-static void appendAsAttribute(llvm::SmallVectorImpl<mlir::Attribute> &attrs,
-                              mlir::Value val) {
-  if (auto *op = val.getDefiningOp()) {
-    if (auto cop = mlir::dyn_cast<mlir::arith::ConstantOp>(op)) {
-      // append the integer constant value
-      if (auto iattr = cop.value().dyn_cast<mlir::IntegerAttr>()) {
-        attrs.push_back(iattr);
-        return;
-      }
-    } else if (auto fld = mlir::dyn_cast<fir::FieldIndexOp>(op)) {
-      if constexpr (AllowFields) {
-        // append the field name and the record type
-        attrs.push_back(fld.field_idAttr());
-        attrs.push_back(fld.on_typeAttr());
-        return;
-      }
-    }
-  }
-  llvm::report_fatal_error("cannot build Op with these arguments");
-}
-
-template <bool AllowFields = true>
-static mlir::ArrayAttr collectAsAttributes(mlir::MLIRContext *ctxt,
-                                           OperationState &result,
-                                           llvm::ArrayRef<mlir::Value> inds) {
-  llvm::SmallVector<mlir::Attribute> attrs;
-  for (auto v : inds)
-    appendAsAttribute<AllowFields>(attrs, v);
-  assert(!attrs.empty());
-  return mlir::ArrayAttr::get(ctxt, attrs);
-}
-
 //===----------------------------------------------------------------------===//
 // GlobalLenOp
 //===----------------------------------------------------------------------===//
 
 mlir::Type fir::GlobalOp::resultType() {
   return wrapAllocaResultType(getType());
 }
 

 }
 
 static void print(mlir::OpAsmPrinter &p, fir::GlobalLenOp &op) {
   p << ' ' << op.getOperation()->getAttr(fir::GlobalLenOp::lenParamAttrName())
     << ", " << op.getOperation()->getAttr(fir::GlobalLenOp::intAttrName());
 }
 
 //===----------------------------------------------------------------------===//
-// ExtractValueOp
-//===----------------------------------------------------------------------===//
-
-void fir::ExtractValueOp::build(mlir::OpBuilder &builder,
-                                OperationState &result, mlir::Type resTy,
-                                mlir::Value aggVal,
-                                llvm::ArrayRef<mlir::Value> inds) {
-  auto aa = collectAsAttributes<>(builder.getContext(), result, inds);
-  build(builder, result, resTy, aggVal, aa);
-}
-
-//===----------------------------------------------------------------------===//
 // FieldIndexOp
 //===----------------------------------------------------------------------===//
 
 static mlir::ParseResult parseFieldIndexOp(mlir::OpAsmParser &parser,
                                            mlir::OperationState &result) {
   llvm::StringRef fieldName;
   auto &builder = parser.getBuilder();
   mlir::Type recty;
void fir::FieldIndexOp::build(mlir::OpBuilder &builder,
   result.addAttribute(typeAttrName(), TypeAttr::get(recTy));
   result.addOperands(operands);
 }
 
 //===----------------------------------------------------------------------===//
 // InsertOnRangeOp
 //===----------------------------------------------------------------------===//
 
-void fir::InsertOnRangeOp::build(mlir::OpBuilder &builder,
-                                 OperationState &result, mlir::Type resTy,
-                                 mlir::Value aggVal, mlir::Value eleVal,
-                                 llvm::ArrayRef<mlir::Value> inds) {
-  auto aa = collectAsAttributes<false>(builder.getContext(), result, inds);
-  build(builder, result, resTy, aggVal, eleVal, aa);
-}
-
 /// Range bounds must be nonnegative, and the range must not be empty.
 static mlir::LogicalResult verify(fir::InsertOnRangeOp op) {
   if (op.coor().size() < 2 || op.coor().size() % 2 != 0)
     return op.emitOpError("has uneven number of values in ranges");
   bool rangeIsKnownToBeNonempty = false;
   for (auto i = op.coor().end(), b = op.coor().begin(); i != b;) {
     int64_t ub = (*--i).cast<IntegerAttr>().getInt();
     int64_t lb = (*--i).cast<IntegerAttr>().getInt();
     if (lb < 0 || ub < 0)
       return op.emitOpError("negative range bound");
     if (rangeIsKnownToBeNonempty)
       continue;
     if (lb > ub)
       return op.emitOpError("empty range");
     rangeIsKnownToBeNonempty = lb < ub;
   }
   return mlir::success();
 }
 
 //===----------------------------------------------------------------------===//
 // InsertValueOp
 //===----------------------------------------------------------------------===//
 
-void fir::InsertValueOp::build(mlir::OpBuilder &builder, OperationState &result,
-                               mlir::Type resTy, mlir::Value aggVal,
-                               mlir::Value eleVal,
-                               llvm::ArrayRef<mlir::Value> inds) {
-  auto aa = collectAsAttributes<>(builder.getContext(), result, inds);
-  build(builder, result, resTy, aggVal, eleVal, aa);
-}
-
 static bool checkIsIntegerConstant(mlir::Attribute attr, int64_t conVal) {
   if (auto iattr = attr.dyn_cast<mlir::IntegerAttr>())
     return iattr.getInt() == conVal;
   return false;
 }
 static bool isZero(mlir::Attribute a) { return checkIsIntegerConstant(a, 0); }
 static bool isOne(mlir::Attribute a) { return checkIsIntegerConstant(a, 1); }