[PATCH] D112961: [fir] Add fir.extract_value and fir.insert_value conversion

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Overall makes sense to me. I would love to see less `auto` - in quite a few places it replaces simple types like `mlir::Type`. I'm rather unfamiliar with this code-base (and MLIR) and it is easier for me to follow the logic when I see the actual type. Especially when translating between dialects (so moving from one namespace to some other namespace).



================
Comment at: flang/lib/Optimizer/CodeGen/DescriptorModel.h:20-33
+//===----------------------------------------------------------------------===//
+// LLVM IR dialect models of C++ types.
+//
+// This supplies a set of model builders to decompose the C declaration of a
+// descriptor (as encoded in ISO_Fortran_binding.h and elsewhere) and
+// reconstruct that type in the LLVM IR dialect.
+//
----------------
Could comments that apply to a whole file be moved to the top? Like here: https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/AffineExpr.h#L7-L12

On a separate note - the TODO suggests that `--target` is unlikely to work correctly. I think that it would help to have this mentioned somewhere, e.g. in https://github.com/llvm/llvm-project/blob/main/flang/docs/RuntimeDescriptor.md. I know that that's not really part of up-streaming, but it might really limit our options in terms of testing. 


================
Comment at: flang/lib/Optimizer/CodeGen/DescriptorModel.h:115
+
+/// Get the type model of the field number `Field` in an ISO descriptor.
+template <int Field>
----------------



================
Comment at: flang/lib/Optimizer/CodeGen/DescriptorModel.h:119
+  Fortran::ISO::Fortran_2018::CFI_cdesc_t dummyDesc{};
+  // check that the descriptor is exactly 8 fields
+  auto [a, b, c, d, e, f, g, h] = dummyDesc;
----------------
The spec for F2018 (18.5.3) lists only 7 fields. Also, nothing is really checked here, is it? The comment probably needs updating.


================
Comment at: flang/lib/Optimizer/CodeGen/TypeConverter.h:66
+  // because it is assumed rank or because we have not determined it yet.
+  static constexpr int unknownRank() { return -1; }
+  // This corresponds to the descriptor as defined ISO_Fortran_binding.h and the
----------------
awarzynski wrote:
> 
Why wrap this in a method as opposed to having a constant?


================
Comment at: flang/lib/Optimizer/CodeGen/TypeConverter.h:66-67
+  // because it is assumed rank or because we have not determined it yet.
+  static constexpr int unknownRank() { return -1; }
+  // 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:71
+    // (buffer*, ele-size, rank, type-descriptor, attribute, [dims])
+    SmallVector<mlir::Type> parts;
+    mlir::Type ele = box.getEleTy();
----------------
Isn't this just `CFI_cdesc_t`? I'm just confused by `parts` here.


================
Comment at: flang/lib/Optimizer/CodeGen/TypeConverter.h:144
+    }
+    (void)st.setBody(members, /*isPacked=*/false);
+    return st;
----------------
Shouldn't the result be checked instead?


================
Comment at: flang/test/Fir/convert-to-llvm.fir:117
+
+// CHECK-LABEL: func @extract
+// CHECK:         %[[STRUCT:.*]] = llvm.mlir.undef : !llvm.struct<"derived", (f32)>
----------------



================
Comment at: flang/test/Fir/convert-to-llvm.fir:134
+// CHECK-SAME:                           %[[ARR:.*]]: !llvm.array<10 x array<10 x struct<(i32, f32)>>>
+// CHECK: %{{.*}} = llvm.extractvalue %[[ARR]][4 : index, 5 : index, 1 : index] : !llvm.array<10 x array<10 x struct<(i32, f32)>>>
+
----------------
Missing `llvm.return`? (for consistency with other tests)


================
Comment at: flang/test/Fir/convert-to-llvm.fir:153
+// CHECK:         %{{.*}} = llvm.insertvalue %{{.*}}, %[[ARR]][4 : index, 5 : index, 1 : index] : !llvm.array<10 x array<10 x struct<(i32, f32)>>>
+
+// -----
----------------
Missing `llvm.return`? (for consistency with other tests)


================
Comment at: flang/test/Fir/convert-to-llvm.fir:166
+// CHECK-SAME:                     %[[TUPLE:.*]]: !llvm.struct<(i32, f32)>
+// CHECK:         %{{.*}} = llvm.insertvalue %{{.*}}, %[[TUPLE]][1 : index] : !llvm.struct<(i32, f32)>
----------------
Missing `llvm.return`? (for consistency with other tests)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112961