[PATCH] D114148: [fir] Add fircg.ext_embox conversion

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Minor comments and request for two more tests.



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1405-1421
+  inline mlir::Value insertLowerBound(mlir::ConversionPatternRewriter &rewriter,
+                                      mlir::Location loc, mlir::Value dest,
+                                      unsigned dim, mlir::Value lb) const {
+    return insertField(rewriter, loc, dest, {7, dim, 0}, lb);
+  }
+
+  inline mlir::Value insertExtent(mlir::ConversionPatternRewriter &rewriter,
----------------
Nit: add the name for the pos 7.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1525
+    } else {
+      gepOperands.push_back(lowerBound);
+    }
----------------
Nit: else can be removed by moving the lowerBound pushing outside.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1551
+                     mlir::Value step, mlir::Value zero, mlir::Type type) {
+  // auto type = ub.getType();
+  mlir::Value extent = rewriter.create<mlir::LLVM::SubOp>(loc, type, ub, lb);
----------------
Nit: remove commented code.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1633
+        if (fir::hasDynamicSize(seqEleTy)) {
+          if (fir::isa_char(seqEleTy)) {
+            assert(xbox.lenParams().size() == 1);
----------------
Is this case covered in the test? (the char has a dynamic size)


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1651
+    mlir::Value stepExpr;
+    if (hasSubcomp) {
+      // We have a subcomponent. The step value needs to be the number of
----------------
I think we need a test for derived type as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114148