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

[Valentin Clement via Phabricator via llvm-commits]

clementval marked 14 inline comments as done.
clementval 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());
----------------
kiranchandramohan wrote:
> rovka wrote:
> > Do we really need op, or could we just replace all of this with 
> > 
> > ```
> > auto func = thisBlock->getParentOfType<mlir::LLVM::LLVMFuncOp>() 
> > ```
> > ?
> I think such a function does not exist.
@kiranchandramohan is right. Block does not have such function. 


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1323
+                  mlir::ConversionPatternRewriter &rewriter) const override {
+    // There should be no dims on this embox op
+    assert(!embox.getShape());
----------------
awarzynski wrote:
> 1. Why?
> 2. Could you "inject" this comment into the `assert`?
If I'm not wrong embox with shape are first lowered to xembox op and then converted to LLVM IR. 

2. done


================
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
----------------
rovka wrote:
> 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 ? 
That's correct. 


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1219
+// CHECK:         %[[TYPE_CODE:.*]] = llvm.mlir.constant(9 : i32) : i32
+// CHECK:         %[[I64_ELEM_SIZE:.*]] = llvm.sext %3 : i32 to i64
+// CHECK:         %[[DESC0:.*]] = llvm.insertvalue %[[I64_ELEM_SIZE]], %[[DESC]][1 : i32] : !llvm.struct<(ptr<array<100 x i32>>, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}})>
----------------
awarzynski wrote:
> 
Thanks for catching this. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113756