[PATCH] D113756: [fir] Add fir.embox conversion
Diana Picus via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 15 02:32:49 PST 2021
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
More information about the llvm-commits
mailing list