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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
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.
+//
----------------
clementval wrote:
> awarzynski wrote:
> > 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. 
> As the todo mentioned, this will need some work to be done right. This will probably need someone to dig deeper into that.  
Yup, that's fine, but I would really appreciate a note on that that can be found outside the code itself. And, that can be referred inside the code whenever such assumptions are made.

I've created https://llvm.org/PR52418 for this reason. Would you mind adding a link?


================
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;
----------------
clementval wrote:
> awarzynski wrote:
> > The spec for F2018 (18.5.3) lists only 7 fields. Also, nothing is really checked here, is it? The comment probably needs updating.
> This is actually a compile time check. if you remove on element or add one your build will fail. I don't think we are at F2018 yet.
> I don't think we are at F2018 yet.
No, but this comes from the `Fortran_2018` namespace, that's why I referred to the 2018 spec. This is all fine to, but 8 is a magic number here. Why do we check for 8 rather than 7 or 13 fields? Could add any sort of hint that would demystify this magic number?


================
Comment at: flang/lib/Optimizer/CodeGen/TypeConverter.h:51
+  // fir.type<name(p : TY'...){f : TY...}>  -->  llvm<"%name = { ty... }">
+  mlir::Type convertRecordType(fir::RecordType derived) {
+    auto name = derived.getName();
----------------
How about a test in "types-to-llvm.fir" for this conversion?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112961