Skip to content

Commit ce3b639

Browse files
authored
[Codegen][Tuner] solve name conflicts for merging td specs (iree-org#22409)
**Follow-up fix for:** iree-org#20127 **Fixes:** iree-org#20673, nod-ai/amd-shark-ai#2440 This issue was discovered while tuning BOO, where the same naming conflict pattern caused the pass to fail. ## Problem When a module contains both a conflicting name and a suffixed variant of that name, the pass could create duplicate names during conflict resolution. ### Example ```mlir module { // First module @apply_op_config } module { // Second module @apply_op_config // conflicts → would rename to @apply_op_config_0 @apply_op_config_0 // but this already exists! → BUG } ``` The second module's `@apply_op_config` would be renamed to `@apply_op_config_0`, but that name already exists in the same module, creating a duplicate. ## Solution This PR checks the newly generated name against `seenNames` and increments the numeric suffix until finding a unique name: - `@apply_op_config` → `@apply_op_config_1` (skipping `_0` since it exists) The fix uses a simple numeric suffix scheme (e.g., `_0`, `_1`, `_2`) instead of module prefixes for cleaner and more maintainable code. --------- Signed-off-by: Bangtian Liu <[email protected]>
1 parent 0e46977 commit ce3b639

File tree

3 files changed

+69
-39
lines changed

3 files changed

+69
-39
lines changed

compiler/plugins/target/ROCM/test/default_tuning_specs_amdgpu.mlir

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@
7575
// MERGE: transform.foreach_match
7676
// MERGE: @match_mmt -> @apply_op_config,
7777
// MERGE-NEXT: @match_attention_2x10x4096x64x64x64_f16 -> @apply_attn_op_config,
78-
// MERGE-NEXT: @match_mmt_2048x1280x5120_f16_f16_f32 -> @iree_default_tuning_spec_gfx942_1_apply_op_config
78+
// MERGE-NEXT: @match_mmt_2048x1280x5120_f16_f16_f32 -> @apply_op_config_0
7979

8080
// NOTE: The order matters above because `foreach_match` ops performs matching from top to bottom.
8181

compiler/src/iree/compiler/Codegen/Common/LinkTuningSpecsPass.cpp

Lines changed: 13 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -143,38 +143,22 @@ static TuningSpecsToMerge collectTuningSpecsToMerge(ModuleOp module) {
143143
}
144144

145145
// Renames a `NamedSequenceOp` to resolve name conflicts caused by merging
146-
// tuning specs.
147-
// The name conflict resolution strategy follows below two rules:
148-
// 1. If the `NamedSequenceOp` is inside a module with a valid symbol name,
149-
// its new name is prefixed with its containing module's symbol name.
150-
// 2. If the module has no symbol name, an incrementing counter is used
151-
// to generate a unique prefix (e.g., `m0_`, `m1_`, etc.).
146+
// tuning specs by appending a numeric suffix until a unique name is found.
152147
static void updateNamedSequenceOp(
153148
NamedSequenceOp op, OpBuilder &builder,
154149
llvm::DenseMap<NamedSequenceOp, ForeachMatchOp> &namedSequenceToUser,
155-
llvm::DenseMap<ModuleOp, std::string> &unnamedModuleNames,
156-
unsigned &unnamedModuleCounter) {
150+
llvm::DenseSet<StringRef> &seenNames) {
157151
StringRef specName = op.getSymName();
158-
ModuleOp parentModule = op->getParentOfType<ModuleOp>();
159-
assert(parentModule);
160-
StringAttr parentSymbol = parentModule.getSymNameAttr();
161-
std::string moduleName;
162-
if (parentSymbol) {
163-
moduleName = parentSymbol.getValue().str();
164-
} else {
165-
if (unnamedModuleNames.contains(parentModule)) {
166-
moduleName = unnamedModuleNames[parentModule];
167-
} else {
168-
std::string newModuleName =
169-
llvm::formatv("m{}", unnamedModuleCounter).str();
170-
++unnamedModuleCounter;
171-
unnamedModuleNames[parentModule] = newModuleName;
172-
moduleName = newModuleName;
173-
}
152+
153+
// Ensure the name is unique by appending a numeric suffix if needed.
154+
std::string uniqueNewSpecName = specName.str();
155+
for (unsigned suffix = 0; seenNames.contains(uniqueNewSpecName); ++suffix) {
156+
uniqueNewSpecName = llvm::formatv("{}_{}", specName, suffix).str();
174157
}
175158

176-
std::string newSpecName = llvm::formatv("{}_{}", moduleName, specName).str();
177-
op.setSymName(newSpecName);
159+
op.setSymName(uniqueNewSpecName);
160+
StringRef newSeqName = op.getSymName();
161+
seenNames.insert(newSeqName);
178162

179163
// Skip updating ForeachMatchOp if the NamedSequenceOp is not used in it.
180164
if (!namedSequenceToUser.contains(op))
@@ -187,7 +171,7 @@ static void updateNamedSequenceOp(
187171
auto getUpdatedSymbol = [&](Attribute attr) -> SymbolRefAttr {
188172
StringRef name = cast<SymbolRefAttr>(attr).getRootReference();
189173
return (name == specName)
190-
? SymbolRefAttr::get(builder.getContext(), newSpecName)
174+
? SymbolRefAttr::get(builder.getContext(), newSeqName)
191175
: cast<SymbolRefAttr>(attr);
192176
};
193177

@@ -236,13 +220,8 @@ static LogicalResult resolveAndMoveNamedSequenceOps(
236220
}
237221

238222
// Update conflicted named sequence ops.
239-
if (!nameConflictOps.empty()) {
240-
llvm::DenseMap<ModuleOp, std::string> unnamedModuleNames;
241-
unsigned unnamedModuleCounter = 0;
242-
for (NamedSequenceOp op : nameConflictOps) {
243-
updateNamedSequenceOp(op, builder, namedSequenceToUser,
244-
unnamedModuleNames, unnamedModuleCounter);
245-
}
223+
for (NamedSequenceOp op : nameConflictOps) {
224+
updateNamedSequenceOp(op, builder, namedSequenceToUser, seenNames);
246225
}
247226

248227
// Move all named sequence ops to the top-level module.

compiler/src/iree/compiler/Codegen/Common/test/link_tuning_specs.mlir

Lines changed: 55 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -268,8 +268,8 @@ module @td_module attributes { transform.with_named_sequence } {
268268
// CHECK: @__kernel_config(
269269
// CHECK: transform.foreach_match
270270
// CHECK: @match -> @apply_op_config
271-
// CHECK: @inner_module_b_match -> @inner_module_b_apply_op_config
272-
// CHECK: @inner_module_c_match -> @apply_op_config_1
271+
// CHECK: @match_0 -> @apply_op_config_0
272+
// CHECK: @match_1 -> @apply_op_config_1
273273

274274
// -----
275275

@@ -338,5 +338,56 @@ module @td_module attributes { transform.with_named_sequence } {
338338
// CHECK: @__kernel_config(
339339
// CHECK: transform.foreach_match
340340
// CHECK: @match -> @apply_op_config
341-
// CHECK: @m0_match -> @m0_apply_op_config
342-
// CHECK: @m1_match -> @apply_op_config_1
341+
// CHECK: @match_0 -> @apply_op_config_0
342+
// CHECK: @match_1 -> @apply_op_config_1
343+
344+
// -----
345+
346+
// Test secondary conflict: when @apply_op_config from the second module conflicts
347+
// and would be renamed to @apply_op_config_0, but @apply_op_config_0 already
348+
// exists in the same module, it should skip to @apply_op_config_1.
349+
350+
module @td_module_secondary_name_conflict attributes {transform.with_named_sequence} {
351+
module attributes {iree_codegen.tuning_spec_with_default_entrypoint, transform.with_named_sequence} {
352+
transform.named_sequence @apply_op_config(%arg0: !transform.any_op {transform.readonly}) {
353+
transform.yield
354+
}
355+
transform.named_sequence @match_a(%arg0: !transform.any_op {transform.readonly}) -> (!transform.any_op) {
356+
transform.yield %arg0 : !transform.any_op
357+
}
358+
transform.named_sequence @__kernel_config(%arg0: !transform.any_op {transform.consumed}) -> !transform.any_op
359+
attributes {iree_codegen.tuning_spec_entrypoint} {
360+
%updated_root = transform.foreach_match in %arg0
361+
@match_a -> @apply_op_config : (!transform.any_op) -> !transform.any_op
362+
transform.yield %updated_root : !transform.any_op
363+
}
364+
}
365+
module attributes {iree_codegen.tuning_spec_with_default_entrypoint, transform.with_named_sequence} {
366+
transform.named_sequence @apply_op_config(%arg0: !transform.any_op {transform.readonly}) {
367+
transform.yield
368+
}
369+
transform.named_sequence @apply_op_config_0(%arg0: !transform.any_op {transform.readonly}) {
370+
transform.yield
371+
}
372+
transform.named_sequence @match_b(%arg0: !transform.any_op {transform.readonly}) -> (!transform.any_op) {
373+
transform.yield %arg0 : !transform.any_op
374+
}
375+
transform.named_sequence @match_c(%arg0: !transform.any_op {transform.readonly}) -> (!transform.any_op) {
376+
transform.yield %arg0 : !transform.any_op
377+
}
378+
transform.named_sequence @__kernel_config(%arg0: !transform.any_op {transform.consumed}) -> !transform.any_op
379+
attributes {iree_codegen.tuning_spec_entrypoint} {
380+
%updated_root = transform.foreach_match in %arg0
381+
@match_b -> @apply_op_config,
382+
@match_c -> @apply_op_config_0 : (!transform.any_op) -> !transform.any_op
383+
transform.yield %updated_root : !transform.any_op
384+
}
385+
}
386+
}
387+
388+
// CHECK-LABEL: module @td_module_secondary_name_conflict
389+
// CHECK: @__kernel_config(
390+
// CHECK: transform.foreach_match
391+
// CHECK: @match_a -> @apply_op_config
392+
// CHECK: @match_b -> @apply_op_config_1
393+
// CHECK: @match_c -> @apply_op_config_0

0 commit comments

Comments
 (0)