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

[Valentin Clement via Phabricator via llvm-commits]

clementval marked 11 inline comments as done.
clementval added inline comments.


================
Comment at: flang/include/flang/Optimizer/Support/TypeCode.h:28
+  // clang-format off
+  switch (bits) {
+  case 8:  return CFI_type_char;
----------------
awarzynski wrote:
> [nit] I think that `width` would be more accurate (`bits` could mean the actual data too)
I changed it to `bitwidth` as it is reference in the top comment.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1077
+    rewriter.setInsertionPointToStart(&func.front());
+    auto size = this->genConstantOffset(loc, rewriter, 1);
+    auto al = rewriter.create<mlir::LLVM::AllocaOp>(loc, toTy, size, alignment);
----------------
awarzynski wrote:
> I thought that you would introduce another method to generate constant values? `genConstantOffset` is rather counter-intuitive here. `genConstantVal` would make more sense here.
I did `genI32Constant`.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1180-1194
+    if (auto seqTy = boxEleTy.dyn_cast<fir::SequenceType>())
+      return getSizeAndTypeCode(loc, rewriter, seqTy.getEleTy(), lenParams);
+    if (boxEleTy.isa<fir::RecordType>()) {
+      auto ptrTy = mlir::LLVM::LLVMPointerType::get(
+          this->lowerTy().convertType(boxEleTy));
+      auto nullPtr = rewriter.create<mlir::LLVM::NullOp>(loc, ptrTy);
+      auto one =
----------------
awarzynski wrote:
> [nit] Perhaps move above `doInteger` and other lambdas not needed for these 2 cases? Basically, this function is a bit dense and it's not obvious what the key cases are. Or add comments? 
This actually trigger couple of regression. Adding comments is fine. 


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1235
+                    mlir::Location loc, fir::RecordType recType) const {
+    std::string name = recType.getLoweredName();
+    auto module = box->template getParentOfType<mlir::ModuleOp>();
----------------
awarzynski wrote:
> What's this name of? Global descriptor, right?
name (once lowered) of the global for the corresponding type descriptor. 


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1237
+    auto module = box->template getParentOfType<mlir::ModuleOp>();
+    if (auto global = module.template lookupSymbol<fir::GlobalOp>(name)) {
+      auto ty = mlir::LLVM::LLVMPointerType::get(
----------------
awarzynski wrote:
> This checks for the global symbol in FIR?
Yes it looks in the fir.global ops that have not been converted yet.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1243
+    }
+    if (auto global =
+            module.template lookupSymbol<mlir::LLVM::GlobalOp>(name)) {
----------------
awarzynski wrote:
> This checks for the global symbol in LLVM?
Yeah we check in the llvm global since it might have been converted already. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113756