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

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:33
+bool allConstants(mlir::ValueRange operands) {
+  for (auto opnd : operands) {
+    if (auto *defop = opnd.getDefiningOp())
----------------
mehdi_amini wrote:
> 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);
> });
> ```
> 
> 
We could use the proposed code. Just removed it now since it is not used currently after removing the assert. 


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:72
+                  mlir::ConversionPatternRewriter &rewriter) const final {
+    mlir::Type ty = this->convertType(op.getType());
+    return doRewrite(op, ty, adaptor, rewriter);
----------------
mehdi_amini wrote:
> Seem like this line is the entire justification for this class? Is this enough for it be pulling its weight?
> 
> Previously added pattern (`GlobalOpConversion`) started with `    auto tyAttr = convertType(global.getType());` ; should it inherit from this class as well or do I miss something?
Yeah `GlobalOpConversion` might inherit from this. I'll explore that and open a separate revision if needed. I would prefer to differ any refactoring (keeping or removing this class) after we have the full picture on this pass. 


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:210
+    return {};
+  }
+
----------------
mehdi_amini wrote:
> 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.
No needed now so will bring it back later. 


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


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


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:269
+  }
+};
+
----------------
mehdi_amini wrote:
> Actually the entire class seems unused?
Removed for now


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:285
+      subscripts[i - 1] = 0;
+    }
+  }
----------------
mehdi_amini wrote:
> 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).
When subscripts is equal to ubounds we reach the end of the array and end of the insertion. 


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:291
+            mlir::ConversionPatternRewriter &rewriter) const override {
+    assert(fir::allConstants(adaptor.getOperands().drop_front(2)));
+
----------------
mehdi_amini wrote:
> 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?
Removed it since this assert does not really makes sense anymore. Looks like it was useful previously when we had more operands. 


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:320
+
+    auto subscripts(lBounds);
+    auto loc = range.getLoc();
----------------
mehdi_amini wrote:
> `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;`
Somehow I missed your last comment on D111288. I'll check with the others what we will do. It seems a bit overkill to go through a RFC for this. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112896