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

[Valentin Clement via Phabricator via llvm-commits]

clementval marked 4 inline comments as done.
clementval added a comment.

Put TODO back in place



================
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,
----------------
kiranchandramohan wrote:
> Nit: add the name for the pos 7.
updated. Added also some constexpr for the values in [dims].


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


================
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
----------------
kiranchandramohan wrote:
> I think we need a test for derived type as well.
Type conversion make it fails before we can reach the pattern 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114148