[PATCH] D113288: [fir] Add fir.box type conversion

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
Comment at: flang/lib/Optimizer/CodeGen/TypeConverter.h:75
+
+  // This corresponds to the descriptor as defined ISO_Fortran_binding.h and the
+  // addendum defined in descriptor.h.
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/TypeConverter.h:78
+  mlir::Type convertBoxType(BoxType box, int rank = unknownRank()) {
+    // (buffer*, ele-size, rank, type-descriptor, attribute, [dims])
+    SmallVector<mlir::Type> parts;
----------------
Looking at https://github.com/llvm/llvm-project/blob/main/flang/include/flang/ISO_Fortran_binding.h#L125-L139, I think that this is incorrect/out-of-date. My suggestion is based on the current implementation in ISO_Fortran_binding.h.


================
Comment at: flang/lib/Optimizer/CodeGen/TypeConverter.h:79
+    // (buffer*, ele-size, rank, type-descriptor, attribute, [dims])
+    SmallVector<mlir::Type> parts;
+    mlir::Type ele = box.getEleTy();
----------------
or `boxTypeParts` or anything a bit more descriptive, pls


================
Comment at: flang/lib/Optimizer/CodeGen/TypeConverter.h:96
+    parts.push_back(getDescFieldTypeModel<6>()(&getContext()));
+    if (rank == unknownRank()) {
+      if (auto seqTy = ele.dyn_cast<SequenceType>())
----------------
IIUC, everything that follows L96 sets up `[dims]`, right? Definitely worth adding a comment.


================
Comment at: flang/test/Fir/types-to-llvm.fir:31
+func private @foo0(%arg0: !fir.box<!fir.array<?xf32>>)
+// CHECK: !llvm.ptr<struct<(ptr<f32>, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, array<1 x array<3 x i{{.*}}>>)>>
+
----------------
Same suggestion for `CHECK-LABEL/CHECK-SAME` as in https://reviews.llvm.org/D113283


================
Comment at: flang/test/Fir/types-to-llvm.fir:34
+func private @foo1(%arg0: !fir.box<!fir.array<10xf32>>)
+// CHECK: !llvm.ptr<struct<(ptr<array<10 x f32>>, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, i{{.*}}, array<1 x array<3 x i{{.*}}>>)>>
+
----------------
IIUC, we won't be able to check the actual type of `i{{.*}}` until https://bugs.llvm.org/show_bug.cgi?id=52418  is resolved. Could you add a comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113288