Update fir.insert_on_range syntax to make the range more explicit (NFC)

[kiranchandramohan]

flang/lib/Optimizer/Dialect/FIROps.cpp

1454–1460

Remove commented code?

[clementval]

flang/lib/Optimizer/Dialect/FIROps.cpp

1460

Why is this removed?

[mehdi_amini]

flang/lib/Optimizer/Dialect/FIROps.cpp

1460

Oh right I forgot to update this: this builder seems conceptually broken to me.
It takes an ArrayRef<mlir::Value> where it expects only constants.
Builders can't fail: so this does not seems like a good API: every caller will have to check that the values are constant before calling this, which will again recheck (but die).
Also the fact that it can be deleted show it isn't used right now.

I'll pull this in another revision and rebase the current one though.

[mehdi_amini]

Rebase/update on builder removal

[kiranchandramohan]

flang/lib/Optimizer/Dialect/FIROps.cpp

1464

Did you miss the comma here?
There might be a missing multi-coordinate test.

1470

upper bounds?

[mehdi_amini]

Fix the multi-dimensional case and add a test for it!

[mehdi_amini]

flang/lib/Optimizer/Dialect/FIROps.cpp

1464

Thanks, good catch :)

[kiranchandramohan]

LGTM. Looks like a good syntax improvement. Also seems to match the example given in https://github.com/llvm/llvm-project/blob/bcad20bc6591c8b503923402038c735a77373f99/flang/include/flang/Optimizer/Dialect/FIROps.td#L2003.

I have a Nit suggestion.

Please wait for @clementval/@schweitz.

flang/lib/Optimizer/Dialect/FIROps.cpp

1445

Nit: The fortran array syntax uses Paren. Might be good to use that. Not a strong opinion. You can wait for other's comments.
i.e Fortran syntax is a(1,3) and not a[1,3].

[mehdi_amini]

flang/lib/Optimizer/Dialect/FIROps.cpp

1445

Happy to adjust as needed!

Something that would be nice to clarify in a doc for the FIR dialect in general are some principles around row/col major and whether indexing starts at 0 or 1, I'm frequently confused.

[mehdi_amini]

flang/lib/Optimizer/Dialect/FIROps.cpp

1445

Happy to adjust as needed!

Thinking about it, I would likely try be to consistently using ( ) for Fortran indexing (col-major, starts at 1) and square brackets for the C-like indexing.
That way the syntax is a bit more than sugar but provides a visual aid on the semantics of the indexing, WDYT?

[kiranchandramohan]

flang/lib/Optimizer/Dialect/FIROps.cpp

1445

That sounds good to me.
WDYT @schweitz @clementval?

I can help with submitting this to fir-dev.

[kiranchandramohan]

BTW @mehdi_amini, is there a way to put up the FIR dialect operations on a webpage just like the MLIR dialects (https://mlir.llvm.org/docs/Dialects/SCFDialect/)?

[clementval]

flang/lib/Optimizer/Dialect/FIROps.cpp

1445

+1 to use Fortran syntax.

[clementval]

Thanks Mehdi for this patch. We welcome changes that improve the readability of the code. Could you consider delaying these kind patches till we have finished upstreaming most of the changes, so as to avoid additional effort to merge this back into fir-dev.
Kiran as volunteered to merge this back to fir-dev now.

[mehdi_amini]

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

2002

Actually this change should likely be separated in another revision from the ASM syntax one.

[clementval]

In D112899#3105919, @clementval wrote:

Thanks Mehdi for this patch. We welcome changes that improve the readability of the code. Could you consider delaying these kind patches till we have finished upstreaming most of the changes, so as to avoid additional effort to merge this back into fir-dev.
Kiran as volunteered to merge this back to fir-dev now.

@mehdi_amini This comment was mainly for any further syntax change in the dialect. This one can go in anytime.

[mehdi_amini]

@mehdi_amini This comment was mainly for any further syntax change in the dialect. This one can go in anytime.

Ah ok, pushing now then.

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

2002

It's too annoying to split up because IndexElementsAttr can't be parsed alone by default, I'll minimize the churn and land this together this time.

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

let description = [{
     Returns a new ssa value with the same type as the original entity.
     The values are inserted at a contiguous range of indices in Fortran
     row-to-column element order as specified by lower and upper bound
     coordinates.
 
     ```mlir
       %a = fir.undefined !fir.array<10x10xf32>
       %c = arith.constant 3.0 : f32
-      %1 = fir.insert_on_range %a, %c, [0 : index, 7 : index, 0 : index, 2 : index] : (!fir.array<10x10xf32>, f32) -> !fir.array<10x10xf32>
+      %1 = fir.insert_on_range %a, %c from [0, 0] to [7, 2] : (!fir.array<10x10xf32>, f32) -> !fir.array<10x10xf32>
     ```
 
     The first 28 elements of %1, with coordinates from (0,0) to (7,2), have
     the value 3.0.
   }];
 
-  let arguments = (ins fir_SequenceType:$seq, AnyType:$val, ArrayAttr:$coor);
+  let arguments = (ins fir_SequenceType:$seq, AnyType:$val, IndexElementsAttr:$coor);
   let results = (outs fir_SequenceType);
 
   let assemblyFormat = [{
-    $seq `,` $val `,` $coor attr-dict `:` functional-type(operands, results)
+    $seq `,` $val custom<CustomRangeSubscript>($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);";

flang/lib/Optimizer/CodeGen/CodeGen.cpp

if (!gr.empty()) {
               insertOp, seqTyAttr, denseAttr);
         }
       }
     }
     rewriter.eraseOp(global);
     return success();
   }
 
-  bool isFullRange(mlir::ArrayAttr indexes, fir::SequenceType seqTy) const {
+  bool isFullRange(mlir::DenseIntElementsAttr indexes,
+                   fir::SequenceType seqTy) const {
     auto extents = seqTy.getShape();
-    if (indexes.size() / 2 != extents.size())
+    if (indexes.size() / 2 != static_cast<int64_t>(extents.size()))
       return false;
+    auto cur_index = indexes.value_begin<int64_t>();
     for (unsigned i = 0; i < indexes.size(); i += 2) {
-      if (indexes[i].cast<IntegerAttr>().getInt() != 0)
+      if (*(cur_index++) != 0)
         return false;
-      if (indexes[i + 1].cast<IntegerAttr>().getInt() != extents[i / 2] - 1)
+      if (*(cur_index++) != extents[i / 2] - 1)
         return false;
     }
     return true;
   }
 
   mlir::LLVM::Linkage convertLinkage(Optional<StringRef> optLinkage) const {
     if (optLinkage.hasValue()) {
       auto name = optLinkage.getValue();

flang/lib/Optimizer/Dialect/FIROps.cpp

 #include "flang/Optimizer/Dialect/FIRType.h"
 #include "flang/Optimizer/Support/Utils.h"
 #include "mlir/Dialect/CommonFolders.h"
 #include "mlir/Dialect/StandardOps/IR/Ops.h"
 #include "mlir/IR/BuiltinOps.h"
 #include "mlir/IR/Diagnostics.h"
 #include "mlir/IR/Matchers.h"
 #include "mlir/IR/PatternMatch.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include <cstdint>
+#include <mlir/IR/BuiltinAttributes.h>
+#include <mlir/IR/OpDefinition.h>
 
 namespace {
 #include "flang/Optimizer/Dialect/CanonicalizationPatterns.inc"
 } // namespace
 using namespace fir;
 
 /// Return true if a sequence type is of some incomplete size or a record type
 /// is malformed or contains an incomplete sequence type. An incomplete sequence
void fir::FieldIndexOp::build(mlir::OpBuilder &builder,
   result.addAttribute(typeAttrName(), TypeAttr::get(recTy));
   result.addOperands(operands);
 }
 
 //===----------------------------------------------------------------------===//
 // InsertOnRangeOp
 //===----------------------------------------------------------------------===//
 
+static ParseResult
+parseCustomRangeSubscript(mlir::OpAsmParser &parser,
+                          mlir::DenseIntElementsAttr &coord) {
+  llvm::SmallVector<int64_t> lbounds;
+  llvm::SmallVector<int64_t> ubounds;
+  if (parser.parseKeyword("from") ||
+      parser.parseCommaSeparatedList(
+          AsmParser::Delimiter::Square,
+          [&] { return parser.parseInteger(lbounds.emplace_back(0)); }) ||
+      parser.parseKeyword("to") ||
+      parser.parseCommaSeparatedList(AsmParser::Delimiter::Square, [&] {
+        return parser.parseInteger(ubounds.emplace_back(0));
+      }))
+    return failure();
+  llvm::SmallVector<int64_t> zippedBounds;
+  for (auto zip : llvm::zip(lbounds, ubounds)) {
+    zippedBounds.push_back(std::get<0>(zip));
+    zippedBounds.push_back(std::get<1>(zip));
+  }
+  coord = mlir::Builder(parser.getContext()).getIndexTensorAttr(zippedBounds);
+  return success();
+}
+
+void printCustomRangeSubscript(mlir::OpAsmPrinter &printer, InsertOnRangeOp op,
+                               mlir::DenseIntElementsAttr coord) {
+  printer << "from [";
+  StringRef comma = "";
+  // Even entries are the lower bounds.
+  for (auto coor : llvm::enumerate(coord.getValues<int64_t>()))
+    if (coor.index() % 2 == 0)
+      printer << comma << coor.value();
+  printer << "] to [";
+  // Odd entries are the lower bounds.
+  for (auto coor : llvm::enumerate(coord.getValues<int64_t>()))
+    if (coor.index() % 2 != 0)
+      printer << comma << coor.value();
+  printer << "]";
+}
+
+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)
+  mlir::DenseIntElementsAttr coor = op.coor();
+  if (coor.size() < 2 || 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();
+  for (auto i = coor.getValues<int64_t>().end(),
+            b = coor.getValues<int64_t>().begin();
+       i != b;) {
+    int64_t ub = (*--i);
+    int64_t lb = (*--i);
     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;
   }

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

 // -----
 
 // Test global with insert_on_range operation covering the full array
 // in initializer region.
 
 fir.global internal @_QEmultiarray : !fir.array<32x32xi32> {
   %c0_i32 = arith.constant 1 : i32
   %0 = fir.undefined !fir.array<32x32xi32>
-  %2 = fir.insert_on_range %0, %c0_i32, [0 : index, 31 : index, 0 : index, 31 : index] : (!fir.array<32x32xi32>, i32) -> !fir.array<32x32xi32>
+  %2 = fir.insert_on_range %0, %c0_i32 from [0, 0] to [31, 31] : (!fir.array<32x32xi32>, i32) -> !fir.array<32x32xi32>
   fir.has_value %2 : !fir.array<32x32xi32>
 }
 
 // CHECK: llvm.mlir.global internal @_QEmultiarray() : !llvm.array<32 x array<32 x i32>> {
 // CHECK:   %[[CST:.*]] = llvm.mlir.constant(dense<1> : vector<32x32xi32>) : !llvm.array<32 x array<32 x i32>>
 // CHECK:   llvm.return %[[CST]] : !llvm.array<32 x array<32 x i32>>
 // CHECK: }

flang/test/Fir/fir-ops.fir

func @test_misc_ops(%arr1 : !fir.ref<!fir.array<?x?xf32>>, %m : index, %n : index, %o : index, %p : index) {
   // CHECK: [[C1_I32:%.*]] = arith.constant 9 : i32
   %i10 = arith.constant 10 : index
   %j20 = arith.constant 20 : index
   %c2 = arith.constant 2 : index
   %c9 = arith.constant 9 : index
   %c1_i32 = arith.constant 9 : i32
 
   // CHECK: [[ARR2:%.*]] = fir.zero_bits !fir.array<10xi32>
-  // CHECK: [[ARR3:%.*]] = fir.insert_on_range [[ARR2]], [[C1_I32]], [2 : index, 9 : index] : (!fir.array<10xi32>, i32) -> !fir.array<10xi32>
+  // CHECK: [[ARR3:%.*]] = fir.insert_on_range [[ARR2]], [[C1_I32]] from [2] to [9] : (!fir.array<10xi32>, i32) -> !fir.array<10xi32>
   // CHECK: fir.call @noret1([[ARR3]]) : (!fir.array<10xi32>) -> ()
   %arr2 = fir.zero_bits !fir.array<10xi32>
-    %arr3 = fir.insert_on_range %arr2, %c1_i32, [2 : index, 9 : index] : (!fir.array<10xi32>, i32) -> !fir.array<10xi32>
+    %arr3 = fir.insert_on_range %arr2, %c1_i32 from [2] to [9] : (!fir.array<10xi32>, i32) -> !fir.array<10xi32>
   fir.call @noret1(%arr3) : (!fir.array<10xi32>) -> ()
 
   // CHECK: [[SHAPE:%.*]] = fir.shape_shift [[INDXM:%.*]], [[INDXN:%.*]], [[INDXO:%.*]], [[INDXP:%.*]] : (index, index, index, index) -> !fir.shapeshift<2>
   // CHECK: [[AV1:%.*]] = fir.array_load [[ARR1]]([[SHAPE]]) : (!fir.ref<!fir.array<?x?xf32>>, !fir.shapeshift<2>) -> !fir.array<?x?xf32>
   // CHECK: [[FVAL:%.*]] = fir.array_fetch [[AV1]], [[I10]], [[J20]] : (!fir.array<?x?xf32>, index, index) -> f32
   // CHECK: [[AV2:%.*]] = fir.array_update [[AV1]], [[FVAL]], [[I10]], [[J20]] : (!fir.array<?x?xf32>, f32, index, index) -> !fir.array<?x?xf32>
   // CHECK: fir.array_merge_store [[AV1]], [[AV2]] to [[ARR1]] : !fir.array<?x?xf32>, !fir.array<?x?xf32>, !fir.ref<!fir.array<?x?xf32>>
   %s = fir.shape_shift %m, %n, %o, %p : (index, index, index, index) -> !fir.shapeshift<2>

flang/test/Fir/invalid.fir

 }
 
 // -----
 
 fir.global internal @_QEmultiarray : !fir.array<32x32xi32> {
   %c0_i32 = arith.constant 1 : i32
   %0 = fir.undefined !fir.array<32x32xi32>
   // expected-error@+1 {{'fir.insert_on_range' op has uneven number of values in ranges}}
-  %2 = fir.insert_on_range %0, %c0_i32, [0 : index, 31 : index, 0 : index] : (!fir.array<32x32xi32>, i32) -> !fir.array<32x32xi32>
+  %2 = "fir.insert_on_range"(%0, %c0_i32) { coor = dense<[0, 31, 0]> : tensor<3xindex> } : (!fir.array<32x32xi32>, i32) -> !fir.array<32x32xi32>
   fir.has_value %2 : !fir.array<32x32xi32>
 }
 
 // -----
 
 fir.global internal @_QEmultiarray : !fir.array<32x32xi32> {
   %c0_i32 = arith.constant 1 : i32
   %0 = fir.undefined !fir.array<32x32xi32>
   // expected-error@+1 {{'fir.insert_on_range' op has uneven number of values in ranges}}
-  %2 = fir.insert_on_range %0, %c0_i32, [0 : index] : (!fir.array<32x32xi32>, i32) -> !fir.array<32x32xi32>
+  %2 = "fir.insert_on_range"(%0, %c0_i32) { coor = dense<[0]> : tensor<1xindex> }  : (!fir.array<32x32xi32>, i32) -> !fir.array<32x32xi32>
   fir.has_value %2 : !fir.array<32x32xi32>
 }
 
 // -----
 
 fir.global internal @_QEmultiarray : !fir.array<32x32xi32> {
   %c0_i32 = arith.constant 1 : i32
   %0 = fir.undefined !fir.array<32x32xi32>
   // expected-error@+1 {{'fir.insert_on_range' op negative range bound}}
-  %2 = fir.insert_on_range %0, %c0_i32, [-1 : index, 0 : index] : (!fir.array<32x32xi32>, i32) -> !fir.array<32x32xi32>
+  %2 = fir.insert_on_range %0, %c0_i32 from [-1] to [0] : (!fir.array<32x32xi32>, i32) -> !fir.array<32x32xi32>
   fir.has_value %2 : !fir.array<32x32xi32>
 }
 
 // -----
 
 fir.global internal @_QEmultiarray : !fir.array<32x32xi32> {
   %c0_i32 = arith.constant 1 : i32
   %0 = fir.undefined !fir.array<32x32xi32>
   // expected-error@+1 {{'fir.insert_on_range' op empty range}}
-  %2 = fir.insert_on_range %0, %c0_i32, [10 : index, 9 : index] : (!fir.array<32x32xi32>, i32) -> !fir.array<32x32xi32>
+  %2 = fir.insert_on_range %0, %c0_i32 from [10] to [9] : (!fir.array<32x32xi32>, i32) -> !fir.array<32x32xi32>
   fir.has_value %2 : !fir.array<32x32xi32>
 }
 
 // -----
 
 func @bad_save_result(%buffer : !fir.ref<!fir.array<?xf64>>, %n :index) {
   %res = fir.call @array_func() : () -> !fir.array<?xf32>
   %shape = fir.shape %n : (index) -> !fir.shape<1>