From 219ab8fe59487d586fe902bd980047554586e0e7 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Tue, 15 Jul 2025 14:18:51 +0300 Subject: [PATCH 1/3] Migrate to Rust 2024. --- Cargo.toml | 2 +- src/cfg.rs | 2 +- src/cfgssa.rs | 2 +- src/print/mod.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 0e72d323..cb48495f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,7 +5,7 @@ repository = "https://github.com/rust-gpu/spirt" homepage = "https://github.com/rust-gpu/spirt" version = "0.4.0" authors = ["SPIR-T developers", "Embark "] -edition = "2021" +edition = "2024" license = "MIT OR Apache-2.0" readme = "README.md" # FIXME(eddyb) should this point to the version built from `git`? diff --git a/src/cfg.rs b/src/cfg.rs index df8ac4c1..dcd22be1 100644 --- a/src/cfg.rs +++ b/src/cfg.rs @@ -82,7 +82,7 @@ impl ControlFlowGraph { pub fn rev_post_order( &self, func_def_body: &FuncDefBody, - ) -> impl DoubleEndedIterator { + ) -> impl DoubleEndedIterator + use<> { let mut post_order = SmallVec::<[_; 8]>::new(); self.traverse_whole_func( func_def_body, diff --git a/src/cfgssa.rs b/src/cfgssa.rs index 8477a2d5..43e723e5 100644 --- a/src/cfgssa.rs +++ b/src/cfgssa.rs @@ -564,7 +564,7 @@ mod data { self.contains(k).then(|| self.entry(k).remove().unwrap()) } - pub fn keys(&self) -> impl Iterator { + pub fn keys(&self) -> impl Iterator + use { let mut i = 0; let mut remaining = self.occupied; iter::from_fn(move || { diff --git a/src/print/mod.rs b/src/print/mod.rs index 0c812bcc..4c037e02 100644 --- a/src/print/mod.rs +++ b/src/print/mod.rs @@ -1952,7 +1952,7 @@ impl AttrsAndDef { let mut maybe_def_end_anchor = pretty::Fragment::default(); let mut name = name.into(); if let [ - pretty::Node::Anchor { is_def: ref mut original_anchor_is_def @ true, anchor, text: _ }, + pretty::Node::Anchor { is_def: original_anchor_is_def @ true, anchor, text: _ }, .., ] = &mut name.nodes[..] { From ef1de53fbdc93f63a3c3556cca777c2764bfc949 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Tue, 15 Jul 2025 14:24:29 +0300 Subject: [PATCH 2/3] Fix new `clippy::manual_is_multiple_of` warnings. --- src/qptr/analyze.rs | 2 +- src/qptr/lift.rs | 2 +- src/spv/read.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qptr/analyze.rs b/src/qptr/analyze.rs index bc496089..eb2754f3 100644 --- a/src/qptr/analyze.rs +++ b/src/qptr/analyze.rs @@ -608,7 +608,7 @@ impl MemTypeLayout { let usage_fixed_len = usage .max_size .map(|size| { - if size % usage_stride.get() != 0 { + if !size.is_multiple_of(usage_stride.get()) { // FIXME(eddyb) maybe this should be propagated up, // as a sign that `usage` is malformed? return Err(()); diff --git a/src/qptr/lift.rs b/src/qptr/lift.rs index 5d568b8c..519d6124 100644 --- a/src/qptr/lift.rs +++ b/src/qptr/lift.rs @@ -245,7 +245,7 @@ impl<'a> LiftToSpvPtrs<'a> { let fixed_len = usage .max_size .map(|size| { - if size % stride.get() != 0 { + if !size.is_multiple_of(stride.get()) { return Err(LiftError(Diag::bug([format!( "DynOffsetBase: size ({size}) not a multiple of stride ({stride})" ) diff --git a/src/spv/read.rs b/src/spv/read.rs index 58178055..fa00111e 100644 --- a/src/spv/read.rs +++ b/src/spv/read.rs @@ -266,7 +266,7 @@ impl ModuleParser { pub fn read_from_spv_bytes(spv_bytes: Vec) -> io::Result { let spv_spec = spec::Spec::get(); - if spv_bytes.len() % 4 != 0 { + if !spv_bytes.len().is_multiple_of(4) { return Err(invalid("not a multiple of 4 bytes")); } // May need to mutate the bytes (to normalize endianness) later below. From 55ee3c8ed3a96beb1ae483a7f92b503dd30bfdcc Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Tue, 15 Jul 2025 14:24:13 +0300 Subject: [PATCH 3/3] Fix new `clippy::collapsible_if` warnings. --- src/cfg.rs | 16 ++++++------ src/passes/link.rs | 26 +++++++++----------- src/print/mod.rs | 26 ++++++++++---------- src/qptr/analyze.rs | 16 ++++++------ src/qptr/lift.rs | 44 ++++++++++++++++----------------- src/qptr/lower.rs | 20 +++++++-------- src/spv/lower.rs | 60 ++++++++++++++++++++++----------------------- src/spv/read.rs | 10 ++++---- 8 files changed, 106 insertions(+), 112 deletions(-) diff --git a/src/cfg.rs b/src/cfg.rs index dcd22be1..2e87a559 100644 --- a/src/cfg.rs +++ b/src/cfg.rs @@ -1871,14 +1871,14 @@ impl<'a> Structurizer<'a> { // but instead make all `Region`s entirely hermetic wrt inputs. #[allow(clippy::manual_flatten)] for case in cases { - if let Ok(ClaimedRegion { structured_body, structured_body_inputs, .. }) = case { - if !structured_body_inputs.is_empty() { - self.region_input_rewrites.insert( - structured_body, - RegionInputRewrites::ReplaceWith(structured_body_inputs), - ); - self.func_def_body.at_mut(structured_body).def().inputs.clear(); - } + if let Ok(ClaimedRegion { structured_body, structured_body_inputs, .. }) = case + && !structured_body_inputs.is_empty() + { + self.region_input_rewrites.insert( + structured_body, + RegionInputRewrites::ReplaceWith(structured_body_inputs), + ); + self.func_def_body.at_mut(structured_body).def().inputs.clear(); } } diff --git a/src/passes/link.rs b/src/passes/link.rs index 68c56b3c..0e4b1774 100644 --- a/src/passes/link.rs +++ b/src/passes/link.rs @@ -95,10 +95,10 @@ impl Visitor<'_> for LiveExportCollector<'_> { match *import { Import::LinkName(name) => { let export_key = ExportKey::LinkName(name); - if let Some(&exportee) = self.module.exports.get(&export_key) { - if self.live_exports.insert(export_key) { - exportee.inner_visit_with(self); - } + if let Some(&exportee) = self.module.exports.get(&export_key) + && self.live_exports.insert(export_key) + { + exportee.inner_visit_with(self); } } } @@ -198,12 +198,11 @@ impl Visitor<'_> for ImportResolutionCollector<'_> { // FIXME(eddyb) if the export is missing (or the wrong kind), it will // simply not get remapped - perhaps some kind of diagnostic is in // order? (maybe an entire pass infrastructure that can report errors) - if let DeclDef::Imported(Import::LinkName(name)) = gv_decl.def { - if let Some(&Exportee::GlobalVar(def_gv)) = + if let DeclDef::Imported(Import::LinkName(name)) = gv_decl.def + && let Some(&Exportee::GlobalVar(def_gv)) = self.module.exports.get(&ExportKey::LinkName(name)) - { - self.resolved_global_vars.insert(gv, def_gv); - } + { + self.resolved_global_vars.insert(gv, def_gv); } } } @@ -215,12 +214,11 @@ impl Visitor<'_> for ImportResolutionCollector<'_> { // FIXME(eddyb) if the export is missing (or the wrong kind), it will // simply not get remapped - perhaps some kind of diagnostic is in // order? (maybe an entire pass infrastructure that can report errors) - if let DeclDef::Imported(Import::LinkName(name)) = func_decl.def { - if let Some(&Exportee::Func(def_func)) = + if let DeclDef::Imported(Import::LinkName(name)) = func_decl.def + && let Some(&Exportee::Func(def_func)) = self.module.exports.get(&ExportKey::LinkName(name)) - { - self.resolved_funcs.insert(func, def_func); - } + { + self.resolved_funcs.insert(func, def_func); } } } diff --git a/src/print/mod.rs b/src/print/mod.rs index 4c037e02..6138992e 100644 --- a/src/print/mod.rs +++ b/src/print/mod.rs @@ -630,13 +630,13 @@ impl<'a> Visitor<'a> for Plan<'a> { } fn visit_func_decl(&mut self, func_decl: &'a FuncDecl) { - if let DeclDef::Present(func_def_body) = &func_decl.def { - if let Some(cfg) = &func_def_body.unstructured_cfg { - for region in cfg.rev_post_order(func_def_body) { - if let Some(control_inst) = cfg.control_inst_on_exit_from.get(region) { - for &target in &control_inst.targets { - *self.use_counts.entry(Use::RegionLabel(target)).or_default() += 1; - } + if let DeclDef::Present(func_def_body) = &func_decl.def + && let Some(cfg) = &func_def_body.unstructured_cfg + { + for region in cfg.rev_post_order(func_def_body) { + if let Some(control_inst) = cfg.control_inst_on_exit_from.get(region) { + for &target in &control_inst.targets { + *self.use_counts.entry(Use::RegionLabel(target)).or_default() += 1; } } } @@ -4089,12 +4089,12 @@ impl Print for FuncAt<'_, DataInst> { } ConstKind::SpvInst { spv_inst_and_const_inputs } => { let (spv_inst, _const_inputs) = &**spv_inst_and_const_inputs; - if spv_inst.opcode == wk.OpConstant { - if let [spv::Imm::Short(_, x)] = spv_inst.imms[..] { - // HACK(eddyb) only allow unambiguously positive values. - if i32::try_from(x).and_then(u32::try_from) == Ok(x) { - return Some(PseudoImm::U32(x)); - } + if spv_inst.opcode == wk.OpConstant + && let [spv::Imm::Short(_, x)] = spv_inst.imms[..] + { + // HACK(eddyb) only allow unambiguously positive values. + if i32::try_from(x).and_then(u32::try_from) == Ok(x) { + return Some(PseudoImm::U32(x)); } } } diff --git a/src/qptr/analyze.rs b/src/qptr/analyze.rs index eb2754f3..5c38d126 100644 --- a/src/qptr/analyze.rs +++ b/src/qptr/analyze.rs @@ -533,10 +533,10 @@ impl MemTypeLayout { // "Fast accept" based on type alone (expected as recursion base case). if let QPtrMemUsageKind::StrictlyTyped(usage_type) | QPtrMemUsageKind::DirectAccess(usage_type) = usage.kind + && usage_offset == 0 + && self.original_type == usage_type { - if usage_offset == 0 && self.original_type == usage_type { - return true; - } + return true; } { @@ -924,11 +924,11 @@ impl<'a> InferUsage<'a> { )); } }; - if data_inst_def.output_type.is_some_and(is_qptr) { - if let Some(usage) = output_usage { - usage_or_err_attrs_to_attach - .push((Value::DataInstOutput(data_inst), usage)); - } + if data_inst_def.output_type.is_some_and(is_qptr) + && let Some(usage) = output_usage + { + usage_or_err_attrs_to_attach + .push((Value::DataInstOutput(data_inst), usage)); } } diff --git a/src/qptr/lift.rs b/src/qptr/lift.rs index 519d6124..0b0668c1 100644 --- a/src/qptr/lift.rs +++ b/src/qptr/lift.rs @@ -430,13 +430,13 @@ impl LiftToSpvPtrInstsInFunc<'_> { // FIXME(eddyb) maybe all this data should be packaged up together in a // type with fields like those of `DeferredPtrNoop` (or even more). let type_of_val_as_spv_ptr_with_layout = |v: Value| { - if let Value::DataInstOutput(v_data_inst) = v { - if let Some(ptr_noop) = self.deferred_ptr_noops.get(&v_data_inst) { - return Ok(( - ptr_noop.output_pointer_addr_space, - ptr_noop.output_pointee_layout.clone(), - )); - } + if let Value::DataInstOutput(v_data_inst) = v + && let Some(ptr_noop) = self.deferred_ptr_noops.get(&v_data_inst) + { + return Ok(( + ptr_noop.output_pointer_addr_space, + ptr_noop.output_pointee_layout.clone(), + )); } let (addr_space, pointee_type) = @@ -685,17 +685,15 @@ impl LiftToSpvPtrInstsInFunc<'_> { loop { if let Components::Elements { stride: layout_stride, elem, fixed_len } = &layout.components + && layout_stride == stride + && Ok(index_bounds.clone()) + == fixed_len + .map(|len| i32::try_from(len.get()).map(|len| 0..len)) + .transpose() { - if layout_stride == stride - && Ok(index_bounds.clone()) - == fixed_len - .map(|len| i32::try_from(len.get()).map(|len| 0..len)) - .transpose() - { - access_chain_inputs.push(data_inst_def.inputs[1]); - layout = elem.clone(); - break; - } + access_chain_inputs.push(data_inst_def.inputs[1]); + layout = elem.clone(); + break; } // FIXME(eddyb) deduplicate with `maybe_adjust_pointer_for_access`. @@ -1094,12 +1092,12 @@ impl Transformer for LiftToSpvPtrInstsInFunc<'_> { if let DataInstKind::QPtr(_) = data_inst_def.kind { lifted = Err(LiftError(Diag::bug(["unimplemented qptr instruction".into()]))); - } else if let Some(ty) = data_inst_def.output_type { - if matches!(self.lifter.cx[ty].kind, TypeKind::QPtr) { - lifted = Err(LiftError(Diag::bug([ - "unimplemented qptr-producing instruction".into(), - ]))); - } + } else if let Some(ty) = data_inst_def.output_type + && matches!(self.lifter.cx[ty].kind, TypeKind::QPtr) + { + lifted = Err(LiftError(Diag::bug([ + "unimplemented qptr-producing instruction".into(), + ]))); } } match lifted { diff --git a/src/qptr/lower.rs b/src/qptr/lower.rs index a44dbc15..409ed07e 100644 --- a/src/qptr/lower.rs +++ b/src/qptr/lower.rs @@ -625,16 +625,16 @@ impl LowerFromSpvPtrInstsInFunc<'_> { ); } } - if let Some(output_type) = data_inst_def.output_type { - if let Some((addr_space, pointee)) = self.lowerer.as_spv_ptr_type(output_type) { - old_and_new_attrs.get_or_insert_with(get_old_attrs).attrs.insert( - QPtrAttr::FromSpvPtrOutput { - addr_space: OrdAssertEq(addr_space), - pointee: OrdAssertEq(pointee), - } - .into(), - ); - } + if let Some(output_type) = data_inst_def.output_type + && let Some((addr_space, pointee)) = self.lowerer.as_spv_ptr_type(output_type) + { + old_and_new_attrs.get_or_insert_with(get_old_attrs).attrs.insert( + QPtrAttr::FromSpvPtrOutput { + addr_space: OrdAssertEq(addr_space), + pointee: OrdAssertEq(pointee), + } + .into(), + ); } if let Some(LowerError(e)) = extra_error { diff --git a/src/spv/lower.rs b/src/spv/lower.rs index 86570f1f..12d5c080 100644 --- a/src/spv/lower.rs +++ b/src/spv/lower.rs @@ -799,12 +799,12 @@ impl Module { Seq::Function }; - if let Some(prev_seq) = seq { - if prev_seq > next_seq { - return Err(invalid(&format!( - "out of order: {next_seq:?} instructions must precede {prev_seq:?} instructions" - ))); - } + if let Some(prev_seq) = seq + && prev_seq > next_seq + { + return Err(invalid(&format!( + "out of order: {next_seq:?} instructions must precede {prev_seq:?} instructions" + ))); } seq = Some(next_seq); @@ -978,26 +978,26 @@ impl Module { local_id_defs.insert(id, local_id_def); } - if let Some(def_map) = &mut cfgssa_def_map { - if let DeclDef::Present(func_def_body) = &func_decl.def { - let current_block = match block_details.last() { - Some((¤t_block, _)) => current_block, - // HACK(eddyb) ensure e.g. `OpFunctionParameter` - // are treated like `OpPhi`s of the entry block. - None => func_def_body.body, - }; + if let Some(def_map) = &mut cfgssa_def_map + && let DeclDef::Present(func_def_body) = &func_decl.def + { + let current_block = match block_details.last() { + Some((¤t_block, _)) => current_block, + // HACK(eddyb) ensure e.g. `OpFunctionParameter` + // are treated like `OpPhi`s of the entry block. + None => func_def_body.body, + }; - if opcode == wk.OpLabel { - // HACK(eddyb) the entry block was already added. - if current_block != func_def_body.body { - def_map.add_block(current_block); - } - continue; + if opcode == wk.OpLabel { + // HACK(eddyb) the entry block was already added. + if current_block != func_def_body.body { + def_map.add_block(current_block); } + continue; + } - if let Some(id) = result_id { - def_map.add_def(current_block, id, result_type.unwrap()); - } + if let Some(id) = result_id { + def_map.add_def(current_block, id, result_type.unwrap()); } } } @@ -1643,14 +1643,12 @@ impl Module { } // Sanity-check the entry block. - if let Some(func_def_body) = func_def_body { - if block_details[&func_def_body.body].phi_count > 0 { - // FIXME(remove) embed IDs in errors by moving them to the - // `let invalid = |...| ...;` closure that wraps insts. - return Err(invalid(&format!( - "in %{func_id}, the entry block contains `OpPhi`s" - ))); - } + if let Some(func_def_body) = func_def_body + && block_details[&func_def_body.body].phi_count > 0 + { + // FIXME(remove) embed IDs in errors by moving them to the + // `let invalid = |...| ...;` closure that wraps insts. + return Err(invalid(&format!("in %{func_id}, the entry block contains `OpPhi`s"))); } } diff --git a/src/spv/read.rs b/src/spv/read.rs index fa00111e..87c27738 100644 --- a/src/spv/read.rs +++ b/src/spv/read.rs @@ -213,11 +213,11 @@ impl InstParser<'_> { self.inst.result_id = def.has_result_id.then(&mut id).transpose()?; } - if let Some(type_id) = self.inst.result_type_id { - if !self.known_ids.contains_key(&type_id) { - // FIXME(eddyb) also check that the ID is a valid type. - return Err(Error::UnknownResultTypeId(type_id)); - } + if let Some(type_id) = self.inst.result_type_id + && !self.known_ids.contains_key(&type_id) + { + // FIXME(eddyb) also check that the ID is a valid type. + return Err(Error::UnknownResultTypeId(type_id)); } for (mode, kind) in def.all_operands() {