[PATCH] D113968: [fir] Add fircg.ext_array_coor conversion
Kiran Chandramohan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 24 04:40:29 PST 2021
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
More information about the llvm-commits
mailing list