[PATCH] D112896: [fir] Add fir.insert_on_range conversion

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Looks good in general, just some minor comments inline.



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:33
+bool allConstants(mlir::ValueRange operands) {
+  for (auto opnd : operands) {
+    if (auto *defop = opnd.getDefiningOp())
----------------
Nit: seems like it could be using `llvm::all_of`:
 ```
return llvm::all_of(operands, [] (Value opnd) { 
  auto *defop = opnd.getDefiningOp();
  return defop && isa<mlir::LLVM::ConstantOp, mlir::arith::ConstantOp>(defop);
});
```

Should this use `isConstantLike`? https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/Matchers.h#L53-L56

 ```
return llvm::all_of(operands, [] (Value opnd) { 
  auto *defop = opnd.getDefiningOp();
  return defop && isConstantLike(defop);
});
```




================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:80
+    llvm_unreachable("derived class must override");
+  }
+};
----------------
If you make it a pure virtual `= 0` it'll be a compile time failure.
(Could also use CRTP here)


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:210
+    return {};
+  }
+
----------------
The usual pattern would look like: https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/Linalg/Transforms/ElementwiseOpFusion.cpp#L1182-L1204

But also this function isn't used here, so I rather remove this until it's needed.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:215
+  static void toRowMajor(SmallVectorImpl<mlir::Attribute> &attrs,
+                         mlir::Type ty) {
+    assert(ty && "type is null");
----------------
Same as above, unused.



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:237
+  collectIndices(mlir::ConversionPatternRewriter &rewriter,
+                 mlir::ArrayAttr arrAttr) {
+    llvm::SmallVector<mlir::Attribute> attrs;
----------------
ditto


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:269
+  }
+};
+
----------------
Actually the entire class seems unused?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:285
+      subscripts[i - 1] = 0;
+    }
+  }
----------------
It is worth documenting that if the subscript is full of zeros then it means we reached the end of the array.
This "wrap around" could also be signaled by a returned LogicalResult (or a bool maybe).


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:291
+            mlir::ConversionPatternRewriter &rewriter) const override {
+    assert(fir::allConstants(adaptor.getOperands().drop_front(2)));
+
----------------
May be worth a message `&& "..."` in the assert on the broken invariant.

Also the `InsertOnRangeOp` op is defined with only two operands, how we can get more?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:311
+    SmallVector<std::uint64_t> lBounds;
+    SmallVector<std::uint64_t> uBounds;
+
----------------
Nit: `std::` is spurious here (you don't use it line 295)
(same below)


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:318
+      uBounds.push_back(upperBound[i - 1].cast<IntegerAttr>().getInt());
+    }
+
----------------
Please fuse this loop with the previous one and eliminate the temporary storage `lowerBound` / `upperBound`.
Unless I'm mistaken this is just an extra indirection that doesn't seem useful here.
(You can iterate on the reverse iterators)


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:320
+
+    auto subscripts(lBounds);
+    auto loc = range.getLoc();
----------------
`auto` really doesn't help here.
Which reminds me: did you follow up on https://reviews.llvm.org/D111288#inline-1072217 ?

Also this seems like another spurious copy here?
If this is just for the sake of the naming, a lighter way to do this may be `auto &subscripts = lBounds;`


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:330
+        subscriptAttrs.push_back(
+            IntegerAttr::get(rewriter.getI64Type(), subscript));
+      mlir::ArrayRef<mlir::Attribute> arrayRef(subscriptAttrs);
----------------
`rewriter.getI64Type` isn't necessarily "cheap", it is quite common to get it into a variable outside of the enclosing loop and reuse it.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:331
+            IntegerAttr::get(rewriter.getI64Type(), subscript));
+      mlir::ArrayRef<mlir::Attribute> arrayRef(subscriptAttrs);
+      lastOp = rewriter.create<mlir::LLVM::InsertValueOp>(
----------------
Do you need this? (In general SmallVector implicitly convert to ArrayRef)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112896/new/

https://reviews.llvm.org/D112896