[Mlir-commits] [mlir] [mlir] fix LLVM type converter for structs (PR #73231)
Craig Topper
llvmlistbot at llvm.org
Thu Nov 23 13:06:39 PST 2023
https://github.com/topperc updated https://github.com/llvm/llvm-project/pull/73231
>From 3c7487bae67801b10a1dc7c63fcb718878940878 Mon Sep 17 00:00:00 2001
From: Alex Zinenko <zinenko at google.com>
Date: Thu, 23 Nov 2023 11:28:22 +0000
Subject: [PATCH 1/3] [mlir] fix LLVM type converter for structs
Existing implementation of the LLVM type converter for LLVM structs
containing incompatible types was attempting to change identifiers of
the struct in case of name clash post-conversion (all identified structs
have different names post-conversion since one cannot change the body of
the struct once initialized). Beyond a trivial error of not updating the
counter in renaming, this approach was broken for recursive structs that
can't be made aware of the renaming and would use the pre-existing
struct with clashing name instead.
For example, given
`!llvm.struct<"_Converted.foo", (struct<"_Converted.foo">, f32)>`
the following type
`!llvm.struct<"foo", (struct<"foo", index>)>`
would incorrectly convert to
```
!llvm.struct<"_Converted_1.foo",
(struct<"_Converted.foo",
(struct<"_Converted.foo">, f32)>)>
```
Remove this incorrect renaming and simply refuse to convert types if it
would lead to identifier clashes for structs with different bodies.
Document the expectation that such generated names are reserved and must
not be present in the input IR of the converter. If we ever actually
need to use handle such cases, this can be achieved by temporarily
renaming structs with reserved identifiers to an unreserved name and
back in a pre/post-processing pass that does _not_ use the type
conversion infra.
---
mlir/docs/TargetLLVMIR.md | 15 ++++++++
.../Conversion/LLVMCommon/TypeConverter.cpp | 34 ++++++++++++-------
2 files changed, 36 insertions(+), 13 deletions(-)
diff --git a/mlir/docs/TargetLLVMIR.md b/mlir/docs/TargetLLVMIR.md
index 73a74c394f2af79..b7acf638be57ff4 100644
--- a/mlir/docs/TargetLLVMIR.md
+++ b/mlir/docs/TargetLLVMIR.md
@@ -290,6 +290,21 @@ memref<2 x vector<4x8 x f32>
Tensor types cannot be converted to the LLVM dialect. Operations on tensors must
be [bufferized](Bufferization.md) before being converted.
+### Conversion of LLVM Container Types with Non-Compatible Element Types
+
+Progressive lowering may result in there existing LLVM container types, such
+as LLVM dialect structures, containing non-compatible types:
+`!llvm.struct<(index)>`. Such types are converted recursively using the rules
+described above.
+
+Identified structures are converted to _new_ structures that have their
+identifiers prefixed with `_Converted.` since the bodies of identified types
+cannot be updated once initialized. Such names are considered _reserved_ and
+must not appear in the input code (in practice, C reserves names starting with
+`_` and a capital, and `.` cannot appear in valid C types anyway). If they do
+and have a different body than the result of the conversion, the type conversion
+will stop.
+
### Calling Conventions
Calling conventions provides a mechanism to customize the conversion of function
diff --git a/mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp b/mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp
index 04496d6b8f63449..8d8f8a0546dba58 100644
--- a/mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp
+++ b/mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp
@@ -86,15 +86,7 @@ LLVMTypeConverter::LLVMTypeConverter(MLIRContext *ctx,
if (type.isIdentified()) {
auto convertedType = LLVM::LLVMStructType::getIdentified(
- type.getContext(), ("_Converted_" + type.getName()).str());
- unsigned counter = 1;
- while (convertedType.isInitialized()) {
- assert(counter != UINT_MAX &&
- "about to overflow struct renaming counter in conversion");
- convertedType = LLVM::LLVMStructType::getIdentified(
- type.getContext(),
- ("_Converted_" + std::to_string(counter) + type.getName()).str());
- }
+ type.getContext(), ("_Converted." + type.getName()).str());
SmallVectorImpl<Type> &recursiveStack = getCurrentThreadRecursiveStack();
if (llvm::count(recursiveStack, type)) {
@@ -110,10 +102,26 @@ LLVMTypeConverter::LLVMTypeConverter(MLIRContext *ctx,
if (failed(convertTypes(type.getBody(), convertedElemTypes)))
return std::nullopt;
- if (failed(convertedType.setBody(convertedElemTypes, type.isPacked())))
- return failure();
- results.push_back(convertedType);
- return success();
+ // If the converted type has not been initialized yet, just set its body
+ // to be the converted arguments and return.
+ if (!convertedType.isInitialized()) {
+ if (failed(
+ convertedType.setBody(convertedElemTypes, type.isPacked()))) {
+ return failure();
+ }
+ results.push_back(convertedType);
+ return success();
+ }
+
+ // If it has been initialized and has the same body, just use it. This
+ // ensures that recursive structs keep being recursive rather than
+ // including a non-updated name.
+ if (TypeRange(convertedType.getBody()) == TypeRange(convertedElemTypes)) {
+ results.push_back(convertedType);
+ return success();
+ }
+
+ return failure();
}
SmallVector<Type> convertedSubtypes;
>From a38e695705073a0b46106fb75611de810c191da5 Mon Sep 17 00:00:00 2001
From: Alex Zinenko <zinenko at google.com>
Date: Thu, 23 Nov 2023 11:48:13 +0000
Subject: [PATCH 2/3] git add test
---
mlir/test/Conversion/LLVMCommon/types.mlir | 23 ++++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 mlir/test/Conversion/LLVMCommon/types.mlir
diff --git a/mlir/test/Conversion/LLVMCommon/types.mlir b/mlir/test/Conversion/LLVMCommon/types.mlir
new file mode 100644
index 000000000000000..0029d27b23b5c6d
--- /dev/null
+++ b/mlir/test/Conversion/LLVMCommon/types.mlir
@@ -0,0 +1,23 @@
+// RUN: mlir-opt %s --convert-func-to-llvm --split-input-file | FileCheck %s
+
+// CHECK: @create_clashes_on_conversion(!llvm.struct<"foo", (index)>)
+func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", (f32)>)
+func.func private @create_clashes_on_conversion(!llvm.struct<"foo", (index)>)
+
+// -----
+
+// CHECK: @merge_on_conversion(!llvm.struct<"_Converted.foo", (i64)>) attributes {sym_visibility = "private"}
+func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", (i64)>)
+func.func private @merge_on_conversion(!llvm.struct<"foo", (index)>)
+
+// -----
+
+// CHECK: @create_clashes_on_conversion_recursive(!llvm.struct<"foo", (!llvm.struct<"foo">, index)>)
+func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", (struct<"_Converted.foo">, f32)>)
+func.func private @create_clashes_on_conversion_recursive(!llvm.struct<"foo", (struct<"foo">, index)>)
+
+// -----
+
+// CHECK: @merge_on_conversion_recursive(!llvm.struct<"_Converted.foo", (struct<"_Converted.foo">, i64)>)
+func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", (struct<"_Converted.foo">, i64)>)
+func.func private @merge_on_conversion_recursive(!llvm.struct<"foo", (struct<"foo">, index)>)
>From 4cf3bac134e45eb14419c3edebcfce0e5b213f97 Mon Sep 17 00:00:00 2001
From: Alex Zinenko <zinenko at google.com>
Date: Thu, 23 Nov 2023 21:05:52 +0000
Subject: [PATCH 3/3] address review
---
mlir/docs/TargetLLVMIR.md | 2 +-
mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp | 9 +++++----
mlir/test/Conversion/LLVMCommon/types.mlir | 12 ++++++++++++
3 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/mlir/docs/TargetLLVMIR.md b/mlir/docs/TargetLLVMIR.md
index b7acf638be57ff4..27a399c520647c4 100644
--- a/mlir/docs/TargetLLVMIR.md
+++ b/mlir/docs/TargetLLVMIR.md
@@ -292,7 +292,7 @@ be [bufferized](Bufferization.md) before being converted.
### Conversion of LLVM Container Types with Non-Compatible Element Types
-Progressive lowering may result in there existing LLVM container types, such
+Progressive lowering may result in there LLVM container types, such
as LLVM dialect structures, containing non-compatible types:
`!llvm.struct<(index)>`. Such types are converted recursively using the rules
described above.
diff --git a/mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp b/mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp
index 8d8f8a0546dba58..3a01795ce3f53e5 100644
--- a/mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp
+++ b/mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp
@@ -113,10 +113,11 @@ LLVMTypeConverter::LLVMTypeConverter(MLIRContext *ctx,
return success();
}
- // If it has been initialized and has the same body, just use it. This
- // ensures that recursive structs keep being recursive rather than
- // including a non-updated name.
- if (TypeRange(convertedType.getBody()) == TypeRange(convertedElemTypes)) {
+ // If it has been initialized, has the same body and packed bit, just use
+ // it. This ensures that recursive structs keep being recursive rather
+ // than including a non-updated name.
+ if (TypeRange(convertedType.getBody()) == TypeRange(convertedElemTypes) &&
+ convertedType.isPacked() == type.isPacked()) {
results.push_back(convertedType);
return success();
}
diff --git a/mlir/test/Conversion/LLVMCommon/types.mlir b/mlir/test/Conversion/LLVMCommon/types.mlir
index 0029d27b23b5c6d..6d5cf562abe024a 100644
--- a/mlir/test/Conversion/LLVMCommon/types.mlir
+++ b/mlir/test/Conversion/LLVMCommon/types.mlir
@@ -21,3 +21,15 @@ func.func private @create_clashes_on_conversion_recursive(!llvm.struct<"foo", (s
// CHECK: @merge_on_conversion_recursive(!llvm.struct<"_Converted.foo", (struct<"_Converted.foo">, i64)>)
func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", (struct<"_Converted.foo">, i64)>)
func.func private @merge_on_conversion_recursive(!llvm.struct<"foo", (struct<"foo">, index)>)
+
+// -----
+
+// CHECK: @create_clashing_pack(!llvm.struct<"foo", packed (!llvm.struct<"foo">, index)>)
+func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", (struct<"_Converted.foo">, i64)>)
+func.func private @create_clashing_pack(!llvm.struct<"foo", packed (struct<"foo">, index)>)
+
+// -----
+
+// CHECK: @merge_on_conversion_pack(!llvm.struct<"_Converted.foo", packed (struct<"_Converted.foo">, i64)>)
+func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", packed (struct<"_Converted.foo">, i64)>)
+func.func private @merge_on_conversion_pack(!llvm.struct<"foo", packed (struct<"foo">, index)>)
More information about the Mlir-commits
mailing list