[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.
Herald added a subscriber: liufengdb.


================
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,
----------------
The convention for type constructors is `getTypenameTy`, and we already have `getStructTy` above.


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


================
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,
----------------
Nit: could you please add empty lines between implementations of different functions here and below to improve readability. Documentation comments are also welcome.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:167
+  }
+  static LLVMType setStructTyBody(LLVMType structType,
+                                  ArrayRef<LLVMType> elements,
----------------
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...


================
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:
> 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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72542