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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan 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>();
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113756