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

[schweitz]

Summary

This is a temporary implementation to support Flang. The LLVM-IR parser will need to be extended in some way to support recursive types. The exact approach here is still a work-in-progress.

Unfortunately, this won't pass roundtrip testing yet. Adding a comment to the test file as a reminder.

[jdoerfert]

mlir/test/Dialect/LLVMIR/roundtrip.mlir

222 ↗

(On Diff #237435)

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.

[ftynse]

mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h

142

The convention for type constructors is getTypenameTy, and we already have getStructTy above.

149

You can use llvm::None as an empty ArrayRef

159

Nit: could you please add empty lines between implementations of different functions here and below to improve readability. Documentation comments are also welcome.

167

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

mlir/test/Dialect/LLVMIR/roundtrip.mlir

222 ↗

(On Diff #237435)

+1

223 ↗

(On Diff #237435)

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.

[schweitz]

mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h

142

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.

149

ok

159

ok

167

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.

mlir/test/Dialect/LLVMIR/roundtrip.mlir

222 ↗

(On Diff #237435)

Can someone provide a pointer to something like this? I'm having a hard time envisioning a test that tests for a TODO item.

[jdoerfert]

mlir/test/Dialect/LLVMIR/roundtrip.mlir

222 ↗

(On Diff #237435)

TODO: This should be llvm.func @recursive_type(!llvm<"%a = type { %a* }">)
CHECK: <here check what it is right now>

[schweitz]

mlir/test/Dialect/LLVMIR/roundtrip.mlir

222 ↗

(On Diff #237435)

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.

223 ↗

(On Diff #237435)

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).

[jdoerfert]

mlir/test/Dialect/LLVMIR/roundtrip.mlir

222 ↗

(On Diff #237435)

I'm confused. You have XXXCHECK check right now, why can't we make that into, or augment, a TODO with some information?

[ftynse]

mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h

142

Acknowledged.

mlir/test/Dialect/LLVMIR/roundtrip.mlir

222 ↗

(On Diff #237435)

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.

223 ↗

(On Diff #237435)

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.

[schweitz]

mlir/test/Dialect/LLVMIR/roundtrip.mlir

223 ↗

(On Diff #237435)

It's problematic to build a functional test without a parser that can read the input. Since there is no way to read the recursive type from the IR file, we can't build it that way, nor can we output anything.

When driving this from C++ code, we can build a recursive type in memory and the pretty-printer *does* emit the LLVM type.

[schweitz]

I'll need someone to help with getting this committed to the mono repo. Thanks in advance.

mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h

static typename std::enable_if<llvm::are_base_of<LLVMType, Args...>::value,
                                  LLVMType>::type
   getStructTy(LLVMType elt1, Args... elts) {
     SmallVector<LLVMType, 8> fields({elt1, elts...});
     return getStructTy(&elt1.getDialect(), fields);
   }
   static LLVMType getVectorTy(LLVMType elementType, unsigned numElements);
   static LLVMType getVoidTy(LLVMDialect *dialect);
 
+  // Creation and setting of LLVM's identified struct types
+  static LLVMType createStructTy(LLVMDialect *dialect,
+                                 ArrayRef<LLVMType> elements,
+                                 Optional<StringRef> name,
+                                 bool isPacked = false);
+
+  static LLVMType createStructTy(LLVMDialect *dialect,
+                                 Optional<StringRef> name) {
+    return createStructTy(dialect, llvm::None, name);
+  }
+
+  static LLVMType createStructTy(ArrayRef<LLVMType> elements,
+                                 Optional<StringRef> name,
+                                 bool isPacked = false) {
+    assert(!elements.empty() &&
+           "This method may not be invoked with an empty list");
+    LLVMType ele0 = elements.front();
+    return createStructTy(&ele0.getDialect(), elements, name, isPacked);
+  }
+
+  template <typename... Args>
+  static typename std::enable_if_t<llvm::are_base_of<LLVMType, Args...>::value,
+                                   LLVMType>
+  createStructTy(StringRef name, LLVMType elt1, Args... elts) {
+    SmallVector<LLVMType, 8> fields({elt1, elts...});
+    Optional<StringRef> opt_name(name);
+    return createStructTy(&elt1.getDialect(), fields, opt_name);
+  }
+
+  static LLVMType setStructTyBody(LLVMType structType,
+                                  ArrayRef<LLVMType> elements,
+                                  bool isPacked = false);
+
+  template <typename... Args>
+  static typename std::enable_if_t<llvm::are_base_of<LLVMType, Args...>::value,
+                                   LLVMType>
+  setStructTyBody(LLVMType structType, LLVMType elt1, Args... elts) {
+    SmallVector<LLVMType, 8> fields({elt1, elts...});
+    return setStructTyBody(structType, fields);
+  }
+
 private:
   friend LLVMDialect;
 
   /// Get an LLVMType with a pre-existing llvm type.
   static LLVMType get(MLIRContext *context, llvm::Type *llvmType);
 
   /// Get an LLVMType with an llvm type that may cause changes to the underlying
   /// llvm context when constructed.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

for (auto elt : elements)
     llvmElements.push_back(elt.getUnderlyingType());
 
   // Lock access to the dialect as this may modify the LLVM context.
   return getLocked(dialect, [=] {
     return llvm::StructType::get(dialect->getLLVMContext(), llvmElements,
                                  isPacked);
   });
 }
+inline static SmallVector<llvm::Type *, 8>
+toUnderlyingTypes(ArrayRef<LLVMType> elements) {
+  SmallVector<llvm::Type *, 8> llvmElements;
+  for (auto elt : elements)
+    llvmElements.push_back(elt.getUnderlyingType());
+  return llvmElements;
+}
+LLVMType LLVMType::createStructTy(LLVMDialect *dialect,
+                                  ArrayRef<LLVMType> elements,
+                                  Optional<StringRef> name, bool isPacked) {
+  StringRef sr = name.hasValue() ? *name : "";
+  SmallVector<llvm::Type *, 8> llvmElements(toUnderlyingTypes(elements));
+  return getLocked(dialect, [=] {
+    auto *rv = llvm::StructType::create(dialect->getLLVMContext(), sr);
+    if (!llvmElements.empty())
+      rv->setBody(llvmElements, isPacked);
+    return rv;
+  });
+}
+LLVMType LLVMType::setStructTyBody(LLVMType structType,
+                                   ArrayRef<LLVMType> elements, bool isPacked) {
+  llvm::StructType *st =
+      llvm::cast<llvm::StructType>(structType.getUnderlyingType());
+  SmallVector<llvm::Type *, 8> llvmElements(toUnderlyingTypes(elements));
+  return getLocked(&structType.getDialect(), [=] {
+    st->setBody(llvmElements, isPacked);
+    return st;
+  });
+}
 LLVMType LLVMType::getVectorTy(LLVMType elementType, unsigned numElements) {
   // Lock access to the dialect as this may modify the LLVM context.
   return getLocked(&elementType.getDialect(), [=] {
     return llvm::VectorType::get(elementType.getUnderlyingType(), numElements);
   });
 }
 LLVMType LLVMType::getVoidTy(LLVMDialect *dialect) {
   return dialect->impl->voidTy;

mlir/test/Dialect/LLVMIR/invalid.mlir

 func @nvvm_invalid_mma_7(%a0 : !llvm<"<2 x half>">, %a1 : !llvm<"<2 x half>">,
                          %b0 : !llvm<"<2 x half>">, %b1 : !llvm<"<2 x half>">,
                          %c0 : !llvm.float, %c1 : !llvm.float, %c2 : !llvm.float, %c3 : !llvm.float,
                          %c4 : !llvm.float, %c5 : !llvm.float, %c6 : !llvm.float, %c7 : !llvm.float) {
   // expected-error@+1 {{expected single result}}
   %0 = nvvm.mma.sync %a0, %a1, %b0, %b1, %c0, %c1, %c2, %c3, %c4, %c5, %c6, %c7 {alayout="col", blayout="row"} : (!llvm<"<2 x half>">, !llvm<"<2 x half>">, !llvm<"<2 x half>">, !llvm<"<2 x half>">, !llvm.float, !llvm.float, !llvm.float, !llvm.float, !llvm.float, !llvm.float, !llvm.float, !llvm.float) -> (!llvm<"{ float, float, float, float, float, float, float, float }">, !llvm.i32)
   llvm.return %0 : (!llvm<"{ float, float, float, float, float, float, float, float }">, !llvm.i32)
 }
+
+// -----
+
+// FIXME: the LLVM-IR dialect should parse mutually recursive types
+// CHECK-LABEL: @recursive_type
+// expected-error@+1 {{expected end of string}}
+llvm.func @recursive_type(%a : !llvm<"%a = type { %a* }">) ->
+                          !llvm<"%a = type { %a* }"> {
+  llvm.return %a : !llvm<"%a = type { %a* }">
+}
+
+