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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Make sense, thanks for all the updates! It seems that `genConstantOffset` is still used to generate a lot of values that are not "offsets".  Are you going to rename it? Otherwise,  just a few minor things.



================
Comment at: flang/include/flang/Optimizer/Support/TypeCode.h:28
+  // clang-format off
+  switch (bits) {
+  case 8:  return CFI_type_char;
----------------
[nit] I think that `width` would be more accurate (`bits` could mean the actual data too)


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1070
+
+  // Generate an alloca of size `size` and cast it to type `toTy`
+  mlir::LLVM::AllocaOp
----------------
1. `size` is hard-coded to 1. Can you update the comment accordingly?
2. Could you document what the return value is?


================
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);
----------------
I thought that you would introduce another method to generate constant values? `genConstantOffset` is rather counter-intuitive here. `genConstantVal` would make more sense here.


================
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 =
----------------
[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? 


================
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>();
----------------
What's this name of? Global descriptor, right?


================
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(
----------------
This checks for the global symbol in FIR?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1243
+    }
+    if (auto global =
+            module.template lookupSymbol<mlir::LLVM::GlobalOp>(name)) {
----------------
This checks for the global symbol in LLVM?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1276
+    auto llvmBoxTy = llvmBoxPtrTy.getElementType();
+    mlir::Value dest = rewriter.create<mlir::LLVM::UndefOp>(loc, llvmBoxTy);
+
----------------
[nit] I think that this value is first and foremost the constructed "descriptor". The fact that it's a "dest"ination when calling `insertField` is IMO just an implementation detail. Perhaps just add a comment that documents "what" this method produces? Otherwise, `dest` is a bit  enigmatic.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1313-1314
+
+  /// If the embox is not in a globalOp body, allocate storage for the box and
+  /// store the value inside. Return the input value otherwise.
+  mlir::Value
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113756