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

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1063-1069
+    auto op = rewriter.getInsertionBlock()->getParentOp();
+    // Order to find the Op in whose entry block the alloca should be inserted.
+    // The parent Op if it is an LLVM Function Op.
+    // The ancestor LLVM Function Op.
+    auto func = mlir::isa<mlir::LLVM::LLVMFuncOp>(op)
+                    ? mlir::cast<mlir::LLVM::LLVMFuncOp>(op)
+                    : op->getParentOfType<mlir::LLVM::LLVMFuncOp>();
----------------
kiranchandramohan wrote:
> awarzynski wrote:
> > OK, my bad, the comment refers to `auto op = rewriter.getInsertionBlock()->getParentOp();` as well.  I was only looking at the comment and the code that follows.
> > 
> > I think that `op` is a misleading name (I've just assumed that it's the operation currently being converted). I suggest updating to something more descriptive.
> > 
> > As for the comment itself - IMHO the bit that confused me (2nd and 3rd line) simply repeats what the code does and does not add much. I would delete it. The first sentence in the comment ("// Order to find the Op in whose entry block the alloca should be inserted.")  can also be deleted if you rename `func` as something more descriptive, e.g. `funcForInsertion` or `funcToInsertAllocaInto`.
> Since this has become controversial, I think we can take this opportunity to convert this code into a function that returns the correct LLVM FUncOp for the alloca to be inserted.
> 
> ```
> getLLVMFuncToInsertAlloca
> 
> ```
> Something similar was suggested by @jeanPerier in the original PR.
> https://github.com/flang-compiler/f18-llvm-project/pull/1066#pullrequestreview-760987822
I think @kiranchandramohan is right. This is better in a separate function and should be easier to understand. I just updated the patch with this change. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113756