Skip to content

Commit 43bc81d

Browse files
authored
[mlir] fix LLVM type converter for structs (#73231)
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.
1 parent e90845f commit 43bc81d

File tree

3 files changed

+72
-13
lines changed

3 files changed

+72
-13
lines changed

mlir/docs/TargetLLVMIR.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,21 @@ memref<2 x vector<4x8 x f32>
290290
Tensor types cannot be converted to the LLVM dialect. Operations on tensors must
291291
be [bufferized](Bufferization.md) before being converted.
292292

293+
### Conversion of LLVM Container Types with Non-Compatible Element Types
294+
295+
Progressive lowering may result in there LLVM container types, such
296+
as LLVM dialect structures, containing non-compatible types:
297+
`!llvm.struct<(index)>`. Such types are converted recursively using the rules
298+
described above.
299+
300+
Identified structures are converted to _new_ structures that have their
301+
identifiers prefixed with `_Converted.` since the bodies of identified types
302+
cannot be updated once initialized. Such names are considered _reserved_ and
303+
must not appear in the input code (in practice, C reserves names starting with
304+
`_` and a capital, and `.` cannot appear in valid C types anyway). If they do
305+
and have a different body than the result of the conversion, the type conversion
306+
will stop.
307+
293308
### Calling Conventions
294309

295310
Calling conventions provides a mechanism to customize the conversion of function

mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -86,15 +86,7 @@ LLVMTypeConverter::LLVMTypeConverter(MLIRContext *ctx,
8686

8787
if (type.isIdentified()) {
8888
auto convertedType = LLVM::LLVMStructType::getIdentified(
89-
type.getContext(), ("_Converted_" + type.getName()).str());
90-
unsigned counter = 1;
91-
while (convertedType.isInitialized()) {
92-
assert(counter != UINT_MAX &&
93-
"about to overflow struct renaming counter in conversion");
94-
convertedType = LLVM::LLVMStructType::getIdentified(
95-
type.getContext(),
96-
("_Converted_" + std::to_string(counter) + type.getName()).str());
97-
}
89+
type.getContext(), ("_Converted." + type.getName()).str());
9890

9991
SmallVectorImpl<Type> &recursiveStack = getCurrentThreadRecursiveStack();
10092
if (llvm::count(recursiveStack, type)) {
@@ -110,10 +102,27 @@ LLVMTypeConverter::LLVMTypeConverter(MLIRContext *ctx,
110102
if (failed(convertTypes(type.getBody(), convertedElemTypes)))
111103
return std::nullopt;
112104

113-
if (failed(convertedType.setBody(convertedElemTypes, type.isPacked())))
114-
return failure();
115-
results.push_back(convertedType);
116-
return success();
105+
// If the converted type has not been initialized yet, just set its body
106+
// to be the converted arguments and return.
107+
if (!convertedType.isInitialized()) {
108+
if (failed(
109+
convertedType.setBody(convertedElemTypes, type.isPacked()))) {
110+
return failure();
111+
}
112+
results.push_back(convertedType);
113+
return success();
114+
}
115+
116+
// If it has been initialized, has the same body and packed bit, just use
117+
// it. This ensures that recursive structs keep being recursive rather
118+
// than including a non-updated name.
119+
if (TypeRange(convertedType.getBody()) == TypeRange(convertedElemTypes) &&
120+
convertedType.isPacked() == type.isPacked()) {
121+
results.push_back(convertedType);
122+
return success();
123+
}
124+
125+
return failure();
117126
}
118127

119128
SmallVector<Type> convertedSubtypes;
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
// RUN: mlir-opt %s --convert-func-to-llvm --split-input-file | FileCheck %s
2+
3+
// CHECK: @create_clashes_on_conversion(!llvm.struct<"foo", (index)>)
4+
func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", (f32)>)
5+
func.func private @create_clashes_on_conversion(!llvm.struct<"foo", (index)>)
6+
7+
// -----
8+
9+
// CHECK: @merge_on_conversion(!llvm.struct<"_Converted.foo", (i64)>) attributes {sym_visibility = "private"}
10+
func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", (i64)>)
11+
func.func private @merge_on_conversion(!llvm.struct<"foo", (index)>)
12+
13+
// -----
14+
15+
// CHECK: @create_clashes_on_conversion_recursive(!llvm.struct<"foo", (!llvm.struct<"foo">, index)>)
16+
func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", (struct<"_Converted.foo">, f32)>)
17+
func.func private @create_clashes_on_conversion_recursive(!llvm.struct<"foo", (struct<"foo">, index)>)
18+
19+
// -----
20+
21+
// CHECK: @merge_on_conversion_recursive(!llvm.struct<"_Converted.foo", (struct<"_Converted.foo">, i64)>)
22+
func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", (struct<"_Converted.foo">, i64)>)
23+
func.func private @merge_on_conversion_recursive(!llvm.struct<"foo", (struct<"foo">, index)>)
24+
25+
// -----
26+
27+
// CHECK: @create_clashing_pack(!llvm.struct<"foo", packed (!llvm.struct<"foo">, index)>)
28+
func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", (struct<"_Converted.foo">, i64)>)
29+
func.func private @create_clashing_pack(!llvm.struct<"foo", packed (struct<"foo">, index)>)
30+
31+
// -----
32+
33+
// CHECK: @merge_on_conversion_pack(!llvm.struct<"_Converted.foo", packed (struct<"_Converted.foo">, i64)>)
34+
func.func private @clashing_struct_name(!llvm.struct<"_Converted.foo", packed (struct<"_Converted.foo">, i64)>)
35+
func.func private @merge_on_conversion_pack(!llvm.struct<"foo", packed (struct<"foo">, index)>)

0 commit comments

Comments
 (0)