[PATCH] D113968: [fir] Add fircg.ext_array_coor conversion

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Some minor comments.



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:126-127
+    auto idxTy = lowerTy().indexType();
+    auto c0 = genConstantOffset(loc, rewriter, 0);
+    auto c7 = genConstantOffset(loc, rewriter, 7);
+    auto dimValue = genConstantIndex(loc, idxTy, rewriter, dim);
----------------
Does 0 and 7 have a name defined elsewhere?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:172-192
+  // Get the element type given an LLVM type that is of the form
+  // [llvm.ptr](llvm.array|llvm.struct)+ and the provided indexes.
+  static mlir::Type getBoxEleTy(mlir::Type type,
+                                llvm::ArrayRef<unsigned> indexes) {
+    if (auto t = type.dyn_cast<mlir::LLVM::LLVMPointerType>())
+      type = t.getElementType();
+    for (auto i : indexes) {
----------------
If this is common code with embox then it might be better to add this patch as a child patch of embox.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1252
 
+/// XArrayCoor is the address arithmetic on a dynamically shaped, etc. array.
+/// (See the static restriction on coordinate_of.) array_coor determines the
----------------
Nit: Is something missing before `etc`?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1338
+      if (coor.subcomponent().empty()) {
+        rewriter.replaceOpWithNewOp<mlir::LLVM::BitcastOp>(coor, baseTy, addr);
+      } else {
----------------
Can we do an early return here and remove the following else?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1345
+          // as below, as the LLVM struct type cannot be statically defined.
+          TODO(loc, "derived type with type parameters");
+        }
----------------
Notify failure.


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1379
+        } else {
+          TODO(loc, "compute size of derived type with type parameters");
+        }
----------------
Notify failure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113968