[PATCH] D72542: [Flang] add a band-aid to support the creation of mutually recursive types when lowering to LLVM IR

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:142
+  // Creation and setting of LLVM's identified struct types
+  static LLVMType createStructTy(LLVMDialect *dialect,
+                                 ArrayRef<LLVMType> elements,
----------------
schweitz wrote:
> ftynse wrote:
> > The convention for type constructors is `getTypenameTy`, and we already have `getStructTy` above.
> This follows the LLVM convention with respect to struct types, which can be two phase. Create a named struct instance to get a reference to it. And secondly, set the fields of the struct. That departs from the operation of a `get`, which returns a finalized type instance.
Acknowledged.


================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:222
+
+// XXXCHECK: llvm.func @recursive_type(!llvm<"%a = type { %a* }">)
+//llvm.func @recursive_type(!llvm<"%a = type { %a* }">)
----------------
jdoerfert wrote:
> schweitz wrote:
> > jdoerfert wrote:
> > > schweitz wrote:
> > > > ftynse wrote:
> > > > > jdoerfert wrote:
> > > > > > A "FIXME/TODO" and an actual `CHECK` might be better here. The FIXME explains the problem and the `CHECK` makes sure we see changes to whatever we are doing.
> > > > > +1
> > > > Can someone provide a pointer to something like this? I'm having a hard time envisioning a test that tests for a TODO item.
> > > ```
> > > TODO: This should be llvm.func @recursive_type(!llvm<"%a = type { %a* }">)
> > > CHECK: <here check what it is right now>
> > > ```
> > Running this test as `mlir-opt %s | mlir-opt | FileCheck %s` would result in the following output.
> > 
> > ```
> > mlir/test/Dialect/LLVMIR/roundtrip.mlir:223:34: error: expected end of string
> > llvm.func @recursive_type(!llvm<"%a = type { %a* }">)
> >                                  ^
> > mlir/test/Dialect/LLVMIR/roundtrip.mlir:3:17: error: CHECK-LABEL: expected string not found in input
> > // CHECK-LABEL: func @ops(%arg0: !llvm.i32, %arg1: !llvm.float)
> >                 ^
> > <stdin>:3:1: note: scanning from here
> > module {
> > ^
> > ```
> > 
> > That seems an undesirable option.
> > 
> > Another approach would be to add a separate test altogether, a test that expects the tool to error and terminate.
> I'm confused. You have XXXCHECK check right now, why can't we make that into, or augment, a TODO with some information?
The problem is that mlir-opt can not parse the IR that was printed. The solution to that is to avoid calling mlir-opt twice.


================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:223
+// XXXCHECK: llvm.func @recursive_type(!llvm<"%a = type { %a* }">)
+//llvm.func @recursive_type(!llvm<"%a = type { %a* }">)
----------------
schweitz wrote:
> ftynse wrote:
> > Could we have test for translating such types to LLVM? Normally, we should be able to parse the recursive type and see it after translation. Somewhere in test/Target/llvmir.mlir would be the right place.
> The title of this diff was meant to reflect the transitory nature of this patch. Parser work needs to be done to finish this. It is still undecided the exact nature of that parser work, as far as I understand.
> 
> This patch was being driven by merging the MLIR dependence into the F18/Flang project which was itself trying to be merged into the LLVM monorepo. It looks like all of these merges are on hold for now, so the urgency to have some sort of solution has been alleviated.
> 
> We still need real support for recursive-types in the LLVM-IR dialect of MLIR at some point (to support Fortran in our case).
> 
Even if it is a transient state, having a self-contained (i.e. not depending on f18) test is a way to demonstrate this state is useful for something and at least partially correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72542