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

[Eric Schweitz via Phabricator via llvm-commits]

schweitz marked 3 inline comments as done.
schweitz 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,
----------------
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.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:149
+    SmallVector<LLVMType, 1> elements;
+    return createStructTy(dialect, elements, name);
+  }
----------------
ftynse wrote:
> You can use llvm::None as an empty ArrayRef
ok


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:159
+  }
+  template <typename... Args>
+  static typename std::enable_if_t<llvm::are_base_of<LLVMType, Args...>::value,
----------------
ftynse wrote:
> Nit: could you please add empty lines between implementations of different functions here and below to improve readability. Documentation comments are also welcome.
ok


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:167
+  }
+  static LLVMType setStructTyBody(LLVMType structType,
+                                  ArrayRef<LLVMType> elements,
----------------
ftynse wrote:
> This looks like it would be better suited as an instance method. `set` also implies the argument is modified, which is not the case here...
That is what is happening here.  In the case of a struct that has a field that points back to itself, the struct instance is instantiated and then the fields are set and use the struct instance.


================
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* }">)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72542