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

[Valentin Clement via Phabricator via llvm-commits]

clementval 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.
+//
----------------
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.  


================
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;
----------------
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.


================
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:
> awarzynski wrote:
> > 
> Why wrap this in a method as opposed to having a constant?
Since it's constexpr does it really matter?


================
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();
----------------
awarzynski wrote:
> Isn't this just `CFI_cdesc_t`? I'm just confused by `parts` here.
There is more than just the 8 fields of the `CFI_cdesc_t` so it isn't really it so I guess `parts` or other name could work as well. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112961