[PATCH] D113756: [fir] Add fir.embox conversion

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1062
+                    ? mlir::cast<mlir::LLVM::LLVMFuncOp>(op)
+                    : op->getParentOfType<mlir::LLVM::LLVMFuncOp>();
+    rewriter.setInsertionPointToStart(&func.front());
----------------
Do we really need op, or could we just replace all of this with 

```
auto func = thisBlock->getParentOfType<mlir::LLVM::LLVMFuncOp>() 
```
?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1088
+  static bool isDerivedType(fir::BoxType boxTy) {
+    return static_cast<bool>(unwrapIfDerived(boxTy));
+  }
----------------
Minor nit: I think comparing with nullptr is clearer than casting to bool (but I'm not sure what's idiomatic for FIR types).


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1163
+      assert(!lenParams.empty());
+      return doCharacter(charWidth, lenParams.back());
+    }
----------------
Is it possible to have more than 1 len param? I think this could use a comment on why we only care about the last one.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1222
+    auto split = recType.getName().split('T');
+    std::string name = (split.first + "E.dt." + split.second).str();
+    auto module = box->template getParentOfType<mlir::ModuleOp>();
----------------
This looks like it could easily get out of sync with lowering. Would it be difficult to move this logic somewhere common?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1239
+    // defined elsewhere (e.g., type defined in a module).
+    // For now, create a extern_weak symbols (will become nullptr if unresolved)
+    // to support generating code without the front-end generated symbols.
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:1218
+// CHECK:         %[[ELEM_SIZE:.*]] = llvm.mlir.constant(4 : i32) : i32
+// CHECK:         %[[TYPE_CODE:.*]] = llvm.mlir.constant(9 : i32) : i32
+// CHECK:         %[[I64_ELEM_SIZE:.*]] = llvm.sext %3 : i32 to i64
----------------
awarzynski wrote:
> Where is `9` coming from? I couldn't find it.
https://github.com/llvm/llvm-project/blob/4119da2f7c5fd21c03f6d50aa1d2af3527a9e90f/flang/include/flang/ISO_Fortran_binding.h#L56 ? 


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1238
+
+// Check `fir.embox` in a `fir.global`. Descriptor created by `fir.embox` 
+// conversion are not generating `alloca` instruction. This test make sure of
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:1239
+// Check `fir.embox` in a `fir.global`. Descriptor created by `fir.embox` 
+// conversion are not generating `alloca` instruction. This test make sure of
+// that.
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:1265
+// CHECK: %[[ATTR_I8:.*]] = llvm.trunc %[[CFI_ATTR_POINTER]] : i32 to i8
+// CHECK: %{{.*}} = llvm.insertvalue %[[ATTR_I8]], %{{.*}}[5 : i32] : !llvm.struct<(ptr<i32>, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}})>
+
----------------
I think we should have some tests in this style for various [elem_size, type_code] combinations, and especially char if possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113756