Skip to content

Commit 2047cef

Browse files
committed
Fix three categories of optimizer extraction bugs
1. Replace ExprType egraph query with term-structure type inference: the ExprType query was unreliable due to e-class merges (`:merge old`) and could return wrong types. Now infer type class directly from the extracted term's head operator (Add->Int, FAdd->Float, Eq->Bool). 2. Use OpLoopMerge for loop detection instead of back-edge heuristics: the old approach only detected loops via OpBranch back-edges, missing loops with OpBranchConditional continue blocks. 3. Add BoolConst handling to resolve_term_to_id: extracted terms with (BoolConst N) operands were silently dropped because the parser didn't recognize them. 4. Classify vector types by component type in collect_type_classes: Op::TypeVector now inherits its component's type class (Bool/Int/Float) so type validation catches vector type mismatches.
1 parent 8e86247 commit 2047cef

2 files changed

Lines changed: 96 additions & 45 deletions

File tree

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

Lines changed: 88 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -190,21 +190,19 @@ pub fn optimize_module_direct(module: &Module) -> Result<Module, EgglogOptError>
190190
}
191191
}
192192

193-
// Detect Branch (for back-edge detection)
194-
if inst.class.opcode == Op::Branch {
195-
if let Some(rspirv::dr::Operand::IdRef(target_label)) = inst.operands.first() {
193+
// Detect loops via OpLoopMerge (canonical loop marker).
194+
// This is more reliable than back-edge detection because it
195+
// catches loops with BranchConditional continue blocks.
196+
if inst.class.opcode == Op::LoopMerge {
197+
if let Some(rspirv::dr::Operand::IdRef(merge_label)) = inst.operands.first() {
196198
let label_map = &func_block_labels[func_idx];
197-
if let Some(&target_idx) = label_map.get(target_label) {
198-
// Back-edge: target is at or before current block
199-
if target_idx <= block_idx {
200-
// This is a loop: target_idx is the header, block_idx is the latch
201-
// Simple loop detection: body is all blocks from header to latch
202-
let body_indices: Vec<usize> = (target_idx..=block_idx).collect();
203-
loop_constructs.push(LoopInfo {
204-
body_block_indices: body_indices,
205-
func_idx,
206-
});
207-
}
199+
if let Some(&merge_idx) = label_map.get(merge_label) {
200+
// Loop body spans from header to just before merge block
201+
let body_indices: Vec<usize> = (block_idx..merge_idx).collect();
202+
loop_constructs.push(LoopInfo {
203+
body_block_indices: body_indices,
204+
func_idx,
205+
});
208206
}
209207
}
210208
}
@@ -814,38 +812,25 @@ pub fn optimize_module_direct(module: &Module) -> Result<Module, EgglogOptError>
814812
if let Some(term) = parse_extract_result(&result_str) {
815813
let mut result_type = ctx.id_to_type.get(&id).copied().unwrap_or(0);
816814

817-
// Query ExprType from the egraph to detect type domain changes.
818-
// If the extracted term changed type domain (e.g., int→bool via
819-
// Gamma simplification), correct the result_type to match.
820-
let expr_type_query = format!("(extract (ExprType id{}))", id);
821-
if let Ok(type_results) = egraph.parse_and_run_program(None, &expr_type_query) {
822-
if !type_results.is_empty() {
823-
let type_str = format!("{}", type_results[0]);
824-
let current_class = type_classes
825-
.get(&result_type)
826-
.copied()
827-
.unwrap_or(TypeClass::Other);
828-
let egraph_class = if type_str.contains("BoolType") {
829-
TypeClass::Bool
830-
} else if type_str.contains("IntType") {
831-
TypeClass::Int
832-
} else if type_str.contains("FloatType") {
833-
TypeClass::Float
834-
} else {
835-
TypeClass::Other
815+
// Infer the correct type domain from the extracted term's head
816+
// operator. This is more reliable than querying ExprType from the
817+
// egraph because e-class merges can make ExprType stale/wrong.
818+
let term_class = infer_type_class_from_term(&term);
819+
if term_class != TypeClass::Other {
820+
let current_class = type_classes
821+
.get(&result_type)
822+
.copied()
823+
.unwrap_or(TypeClass::Other);
824+
if current_class != term_class {
825+
let corrected = match term_class {
826+
TypeClass::Bool => bool_type,
827+
TypeClass::Int => int32_type,
828+
TypeClass::Float => float32_type,
829+
TypeClass::Other => None,
836830
};
837-
if egraph_class != TypeClass::Other && egraph_class != current_class {
838-
// Type domain changed - select correct SPIR-V type
839-
let corrected = match egraph_class {
840-
TypeClass::Bool => bool_type,
841-
TypeClass::Int => int32_type,
842-
TypeClass::Float => float32_type,
843-
TypeClass::Other => None,
844-
};
845-
if let Some(ct) = corrected {
846-
result_type = ct;
847-
ctx.id_to_type.insert(id, ct);
848-
}
831+
if let Some(ct) = corrected {
832+
result_type = ct;
833+
ctx.id_to_type.insert(id, ct);
849834
}
850835
}
851836
}
@@ -2160,9 +2145,67 @@ fn collect_type_classes(module: &Module) -> HashMap<Word, TypeClass> {
21602145
}
21612146
}
21622147
}
2148+
// Classify vector types by their component type (e.g., vec4<f32> -> Float)
2149+
for inst in &module.types_global_values {
2150+
if inst.class.opcode == Op::TypeVector {
2151+
if let (Some(id), Some(rspirv::dr::Operand::IdRef(component_type))) =
2152+
(inst.result_id, inst.operands.first())
2153+
{
2154+
if let Some(&component_class) = classes.get(component_type) {
2155+
classes.insert(id, component_class);
2156+
}
2157+
}
2158+
}
2159+
}
21632160
classes
21642161
}
21652162

2163+
/// Infer the type class of an extracted term from its head operator.
2164+
/// This is more reliable than querying ExprType from the egraph because
2165+
/// e-class merges (via `:merge old`) can produce stale type information.
2166+
fn infer_type_class_from_term(term: &str) -> TypeClass {
2167+
let term = term.trim();
2168+
// Extract the head operator from the term (first word after opening paren)
2169+
let head = if let Some(rest) = term.strip_prefix('(') {
2170+
rest.split_whitespace()
2171+
.next()
2172+
.unwrap_or("")
2173+
.trim_end_matches(')')
2174+
} else {
2175+
// Bare symbol or constant - no type info
2176+
return TypeClass::Other;
2177+
};
2178+
2179+
match head {
2180+
// Integer arithmetic
2181+
"Add" | "Sub" | "Mul" | "Neg" | "SDiv" | "UDiv" | "SRem" | "SMod" | "UMod" | "Shl"
2182+
| "ShrS" | "ShrU" | "BitAnd" | "BitOr" | "BitXor" | "BitNot" | "BitReverse"
2183+
| "BitCount" | "RotL" | "RotR" | "SMin" | "SMax" | "UMin" | "UMax" | "SAbs"
2184+
| "FindILsb" | "FindSMsb" | "FindUMsb" | "ConvertFToS" | "ConvertFToU" | "Const"
2185+
| "Const64" | "BitFieldInsert" | "BitFieldSExtract" | "BitFieldUExtract" => TypeClass::Int,
2186+
2187+
// Float arithmetic
2188+
"FAdd" | "FSub" | "FMul" | "FDiv" | "FNeg" | "FRem" | "FMod" | "FMin" | "FMax" | "FAbs"
2189+
| "FFloor" | "FCeil" | "FRound" | "FTrunc" | "ConvertSToF" | "ConvertUToF" | "Sqrt"
2190+
| "InverseSqrt" | "Exp" | "Exp2" | "Log" | "Log2" | "Pow" | "Sin" | "Cos" | "Tan"
2191+
| "Asin" | "Acos" | "Atan" | "Atan2" | "Sinh" | "Cosh" | "Tanh" | "Asinh" | "Acosh"
2192+
| "Atanh" | "Fma" | "Fract" | "Modf" | "Step" | "FSign" | "Radians" | "Degrees"
2193+
| "FMix" | "SmoothStep" | "FClamp" | "NMin" | "NMax" | "NClamp" | "Length" | "Distance"
2194+
| "Normalize" | "Cross" | "Reflect" | "Refract" | "FaceForward" | "FConst" | "DPdx"
2195+
| "DPdy" | "Fwidth" | "DPdxFine" | "DPdyFine" | "FwidthFine" | "DPdxCoarse"
2196+
| "DPdyCoarse" | "FwidthCoarse" | "Dot" => TypeClass::Float,
2197+
2198+
// Boolean-producing operations
2199+
"Eq" | "Ne" | "SLt" | "SLe" | "SGt" | "SGe" | "ULt" | "ULe" | "UGt" | "UGe" | "FOrdEq"
2200+
| "FOrdNe" | "FOrdLt" | "FOrdLe" | "FOrdGt" | "FOrdGe" | "FUnordEq" | "FUnordNe"
2201+
| "FUnordLt" | "FUnordLe" | "FUnordGt" | "FUnordGe" | "LogNot" | "LogAnd" | "LogOr"
2202+
| "LogEq" | "LogNe" | "IsNan" | "IsInf" | "BoolConst" => TypeClass::Bool,
2203+
2204+
// Type-preserving (Gamma, Select, CopyObject, Sym, etc.) - can't determine from head
2205+
_ => TypeClass::Other,
2206+
}
2207+
}
2208+
21662209
/// What type class does this opcode require for its *result* type?
21672210
fn required_result_type_class(op: Op) -> Option<TypeClass> {
21682211
match op {

rust/spirv-tools-opt/src/direct/parse/util.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,14 @@ pub fn resolve_term_to_id(term: &str, id_map: &HashMap<String, Word>) -> Option<
6767
}
6868
}
6969
}
70+
if let Some(rest) = term.strip_prefix("(BoolConst ") {
71+
if let Some(num_str) = rest.strip_suffix(')') {
72+
if let Ok(value) = num_str.trim().parse::<i64>() {
73+
let const_key = format!("boolconst_{}", value);
74+
return id_map.get(&const_key).copied();
75+
}
76+
}
77+
}
7078

7179
None
7280
}

0 commit comments

Comments
 (0)