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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

I have some Nit comments.



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1801-1804
+    auto indexOps = coor.indices().begin();
+    auto shapeOps = coor.shape().begin();
+    auto shiftOps = coor.shift().begin();
+    auto sliceOps = coor.slice().begin();
----------------
Nit: Since these are all for the `for` loop, would it be better to initialize it in the loop init section?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1891
+
+    // The array was not boxed, so it must be contiguous. off is therefore an
+    // element offset and the base type is kept in the GEP unless the element
----------------
Nit: off or offset?


================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1900
+        auto eleTy = fir::dyn_cast_ptrEleTy(coor.getType());
+        assert(eleTy && "result must be a refence-like type");
+        if (fir::characterWithDynamicLen(eleTy)) {
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/CodeGen.cpp:1901
+        assert(eleTy && "result must be a refence-like type");
+        if (fir::characterWithDynamicLen(eleTy)) {
+          assert(coor.lenParams().size() == 1);
----------------
Nit: I think we need a test for this case.


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1595
+
+// Test `fircg.ext_array_coor` conversion.
+
----------------
Nit: I think we need a test with a box.


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1614
+// CHECK:         %[[OFFSET:.*]] = llvm.add %[[SC]], %[[C0_1]]  : i64
+// CHECK:         %[[PEXT:.*]] = llvm.mul %[[C1]], %[[C0]]  : i64
+// CHECK:         %[[BITCAST:.*]] = llvm.bitcast %[[ARG0]] : !llvm.ptr<i32> to !llvm.ptr<i32>
----------------
Nit: Not used?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1622
+  %c0 = arith.constant 0 : i64
+  %1 = fircg.ext_array_coor %arg0(%c0) origin %c0[%c0, %c0, %c0]<%c0> : (!fir.ref<!fir.array<?xi32>>, i64, i64, i64, i64, i64, i64) -> !fir.ref<i32>
+  return
----------------
Nit: Can we have a non-zero shift and slice? Also can the slice be of a different dimension (3) than the array (1)? Or have I misunderstood?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:1637
+// CHECK:         %[[OFFSET:.*]] = llvm.add %[[STRIDE]], %[[C0_1]]  : i64
+// CHECK:         %[[NEXT_STRIDE:.*]] = llvm.mul %[[C1]], %[[C0]]  : i64
+// CHECK:         %[[BITCAST:.*]] = llvm.bitcast %[[ARG0]] : !llvm.ptr<i32> to !llvm.ptr<i32>
----------------
Nit: Not used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113968