Skip to content

Commit 3d85fc6

Browse files
committed
Connect EffGamma condition to egraph, expand loop guards, resolve aliases globally
Three fixes for CI difftest failures: 1. EffGamma condition: Use actual egraph variable (id{N} or BSym "id{N}") instead of synthetic "cond{N}" disconnected from the value. Fixes incorrect e-class merges and arm swaps in control_flow and math_ops. 2. Loop guard: Check ALL selection blocks (header, then, else, merge) against both loop_block_set and continue_block_set, not just the header. Fixes continue block unreachability in control_flow_complex. 3. resolve_aliases: Extend to cover types_global_values and annotations, not just function body instructions. Fixes undefined id errors in matrix_ops. Test updated to use non-constant condition for merge-return test since the connected condition now correctly enables constant folding.
1 parent d4280cf commit 3d85fc6

2 files changed

Lines changed: 48 additions & 18 deletions

File tree

rust/spirv-tools-opt/src/direct/mod.rs

Lines changed: 45 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -570,21 +570,21 @@ pub fn optimize_module_direct(module: &Module) -> Result<Module, EgglogOptError>
570570

571571
// For each selection construct, convert to RVSDG EffGamma
572572
for (sel_idx, sel) in selection_constructs.iter().enumerate() {
573-
// Skip selection constructs inside loop bodies to avoid breaking continue block reachability
574-
if loop_block_set.contains(&(sel.func_idx, sel.header_block_idx)) {
575-
continue;
576-
}
577-
// Skip selections whose branch targets or merge block overlap with loop continue blocks
573+
// Skip selections that overlap with loop bodies or continue blocks.
574+
// Check ALL blocks involved (header, then, else, merge) — not just the header —
575+
// because SPIR-V blocks may not be laid out contiguously.
578576
{
579577
let label_map = &func_block_labels[sel.func_idx];
580-
let touches_continue = [&sel.then_label, &sel.else_label, &sel.merge_label]
581-
.iter()
582-
.any(|label| {
583-
label_map
584-
.get(label)
585-
.map_or(false, |&idx| continue_block_set.contains(&(sel.func_idx, idx)))
586-
});
587-
if touches_continue {
578+
let in_loop = loop_block_set.contains(&(sel.func_idx, sel.header_block_idx))
579+
|| [&sel.then_label, &sel.else_label, &sel.merge_label]
580+
.iter()
581+
.any(|label| {
582+
label_map.get(label).map_or(false, |&idx| {
583+
loop_block_set.contains(&(sel.func_idx, idx))
584+
|| continue_block_set.contains(&(sel.func_idx, idx))
585+
})
586+
});
587+
if in_loop {
588588
continue;
589589
}
590590
}
@@ -644,10 +644,15 @@ pub fn optimize_module_direct(module: &Module) -> Result<Module, EgglogOptError>
644644

645645
// Create the EffGamma term
646646
let effect_var = format!("eff_sel{}", sel_idx);
647-
// Use BSym for condition (conditions are always boolean).
648-
// We use a synthetic name "cond{N}" to preserve the conditional structure
649-
// so constant folding doesn't eliminate the gamma during saturation.
650-
let cond_sym = format!("(BSym \"cond{}\")", cond_id);
647+
// Use the actual condition variable from the egraph so that
648+
// Gamma simplification rules can properly track arm correspondence.
649+
// A synthetic "cond{N}" would be disconnected from the actual value,
650+
// allowing incorrect e-class merges and arm swaps.
651+
let cond_sym = if ctx.id_to_term.contains_key(&cond_id) {
652+
format!("id{}", cond_id)
653+
} else {
654+
format!("(BSym \"id{}\")", cond_id)
655+
};
651656

652657
let eff_gamma = format!(
653658
"(let {} (EffGamma {} {} {}))",
@@ -1772,6 +1777,29 @@ fn resolve_aliases(module: &mut Module, id_aliases: &HashMap<Word, Word>) {
17721777
final_aliases.insert(from, target);
17731778
}
17741779

1780+
// Resolve aliases in types_global_values (constants, composite constants, etc.)
1781+
for inst in &mut module.types_global_values {
1782+
for op in &mut inst.operands {
1783+
if let Some(ref_id) = op.id_ref_any() {
1784+
if let Some(&target) = final_aliases.get(&ref_id) {
1785+
*op = rspirv::dr::Operand::IdRef(target);
1786+
}
1787+
}
1788+
}
1789+
}
1790+
1791+
// Resolve aliases in annotations (decorations referencing value IDs)
1792+
for inst in &mut module.annotations {
1793+
for op in &mut inst.operands {
1794+
if let Some(ref_id) = op.id_ref_any() {
1795+
if let Some(&target) = final_aliases.get(&ref_id) {
1796+
*op = rspirv::dr::Operand::IdRef(target);
1797+
}
1798+
}
1799+
}
1800+
}
1801+
1802+
// Resolve aliases in function body instructions
17751803
for func in &mut module.functions {
17761804
for block in &mut func.blocks {
17771805
for inst in &mut block.instructions {

rust/spirv-tools-opt/tests/opt_block_cli.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1259,9 +1259,11 @@ fn build_selection_return_merge_module() -> (Vec<u32>, u32, u32, u32, u32, u32)
12591259
let merge_label = b.id();
12601260
let then_label = b.id();
12611261
let else_label = b.id();
1262-
let cond = b.constant_true(bool_ty);
12631262

12641263
b.begin_block(None).unwrap();
1264+
// Use a non-constant condition (comparison of parameters) so constant
1265+
// folding doesn't eliminate the branch — we want to test merge-return.
1266+
let cond = b.s_less_than(bool_ty, None, x, y).unwrap();
12651267
b.selection_merge(merge_label, SelectionControl::NONE)
12661268
.expect("selection merge");
12671269
b.branch_conditional(cond, then_label, else_label, std::iter::empty())

0 commit comments

Comments
 (0)