Replace complex kill_with with a simple Vec::retain

kill_with was also reordering instructions, which in general, should
preferably never be done - declaration order in spir-v is important
enough to try to adhere to that rule.

rspirv-linker/src/import_export_link.rs

@@ -1,8 +1,5 @@
 use crate::ty::trans_aggregate_type;
-use crate::{
-    inst_fully_eq, kill_annotations_and_debug, kill_with, print_type, DefAnalyzer, LinkerError,
-    Options, Result,
-};
+use crate::{inst_fully_eq, operand_idref, print_type, DefAnalyzer, LinkerError, Options, Result};
 use rspirv::spirv;
 use std::collections::HashMap;
 
@@ -180,10 +177,10 @@ pub fn kill_linkage_instructions(
     }
 
     // drop linkage attributes (both import and export)
-    kill_with(&mut module.annotations, |inst| {
+    module.annotations.retain(|inst| {
         let eq = pairs.iter().any(|p| {
             if inst.operands.is_empty() {
-                return false;
+                return true;
             }
 
             if let rspirv::dr::Operand::IdRef(id) = inst.operands[0] {
@@ -193,27 +190,51 @@ pub fn kill_linkage_instructions(
             }
         });
 
-        eq && inst.class.opcode == spirv::Op::Decorate
-            && inst.operands[1]
-                == rspirv::dr::Operand::Decoration(spirv::Decoration::LinkageAttributes)
+        !eq || inst.class.opcode != spirv::Op::Decorate
+            || inst.operands[1]
+                != rspirv::dr::Operand::Decoration(spirv::Decoration::LinkageAttributes)
     });
 
     if !opts.lib {
-        kill_with(&mut module.annotations, |inst| {
-            inst.class.opcode == spirv::Op::Decorate
-                && inst.operands[1]
-                    == rspirv::dr::Operand::Decoration(spirv::Decoration::LinkageAttributes)
-                && inst.operands[3] == rspirv::dr::Operand::LinkageType(spirv::LinkageType::Export)
+        module.annotations.retain(|inst| {
+            inst.class.opcode != spirv::Op::Decorate
+                || inst.operands[1]
+                    != rspirv::dr::Operand::Decoration(spirv::Decoration::LinkageAttributes)
+                || inst.operands[3] != rspirv::dr::Operand::LinkageType(spirv::LinkageType::Export)
         });
     }
 
     // drop OpCapability Linkage
-    kill_with(&mut module.capabilities, |inst| {
-        inst.class.opcode == spirv::Op::Capability
-            && inst.operands[0] == rspirv::dr::Operand::Capability(spirv::Capability::Linkage)
+    module.capabilities.retain(|inst| {
+        inst.class.opcode != spirv::Op::Capability
+            || inst.operands[0] != rspirv::dr::Operand::Capability(spirv::Capability::Linkage)
     })
 }
 
+fn kill_with_first_id(insts: &mut Vec<rspirv::dr::Instruction>, id: u32) {
+    insts.retain(|inst| {
+        if inst.operands.is_empty() {
+            return true;
+        }
+
+        matches!(operand_idref(&inst.operands[0]), Some(w) if w != id)
+    })
+}
+
+fn kill_annotations_and_debug(module: &mut rspirv::dr::Module, id: u32) {
+    kill_with_first_id(&mut module.annotations, id);
+
+    // need to remove OpGroupDecorate members that mention this id
+    module.annotations.iter_mut().for_each(|inst| {
+        if inst.class.opcode == spirv::Op::GroupDecorate {
+            inst.operands
+                .retain(|op| matches!(op, rspirv::dr::Operand::IdRef(w) if *w != id));
+        }
+    });
+
+    kill_with_first_id(&mut module.debugs, id);
+}
+
 pub fn import_kill_annotations_and_debug(module: &mut rspirv::dr::Module, info: &LinkInfo) {
     for import in &info.imports {
         kill_annotations_and_debug(module, import.id);

rspirv-linker/src/lib.rs

@@ -90,53 +90,6 @@ fn replace_all_uses_with(module: &mut rspirv::dr::Module, before: u32, after: u3
     });
 }
 
-fn kill_with_id(insts: &mut Vec<rspirv::dr::Instruction>, id: u32) {
-    kill_with(insts, |inst| {
-        if inst.operands.is_empty() {
-            return false;
-        }
-
-        matches!(operand_idref(&inst.operands[0]), Some(w) if w == id)
-    })
-}
-
-fn kill_with<F>(insts: &mut Vec<rspirv::dr::Instruction>, f: F)
-where
-    F: Fn(&rspirv::dr::Instruction) -> bool,
-{
-    if insts.is_empty() {
-        return;
-    }
-
-    let mut idx = insts.len() - 1;
-    // odd backwards loop so we can swap_remove
-    loop {
-        if f(&insts[idx]) {
-            insts.swap_remove(idx);
-        }
-
-        if idx == 0 || insts.is_empty() {
-            break;
-        }
-
-        idx -= 1;
-    }
-}
-
-fn kill_annotations_and_debug(module: &mut rspirv::dr::Module, id: u32) {
-    kill_with_id(&mut module.annotations, id);
-
-    // need to remove OpGroupDecorate members that mention this id
-    module.annotations.iter_mut().for_each(|inst| {
-        if inst.class.opcode == spirv::Op::GroupDecorate {
-            inst.operands
-                .retain(|op| matches!(op, rspirv::dr::Operand::IdRef(w) if *w != id));
-        }
-    });
-
-    kill_with_id(&mut module.debugs, id);
-}
-
 fn inst_fully_eq(a: &rspirv::dr::Instruction, b: &rspirv::dr::Instruction) -> bool {
     // both function instructions need to be 100% identical so check all members
     // jb-todo: derive(PartialEq) on Instruction?

rspirv-linker/src/test.rs

@@ -416,18 +416,18 @@ fn use_exported_func_param_attr() -> Result<()> {
 
     let expect = r#"OpCapability Kernel
         OpModuleProcessed "Linked by rspirv-linker"
-        OpDecorate %1 FuncParamAttr Sext
-        OpDecorate %2 FuncParamAttr Zext
-        %2 = OpDecorationGroup
-        OpGroupDecorate %2 %3
+        OpDecorate %1 FuncParamAttr Zext
+        %1 = OpDecorationGroup
+        OpGroupDecorate %1 %2
+        OpDecorate %3 FuncParamAttr Sext
         %4 = OpTypeVoid
         %5 = OpTypeInt 32 0
         %6 = OpTypeFunction %4 %5
         %7 = OpFunction %4 None %6
-        %3 = OpFunctionParameter %5
+        %2 = OpFunctionParameter %5
         OpFunctionEnd
         %8 = OpFunction %4 None %6
-        %1 = OpFunctionParameter %5
+        %3 = OpFunctionParameter %5
         %9 = OpLabel
         OpReturn
         OpFunctionEnd"#;
@@ -483,14 +483,14 @@ fn names_and_decorations() -> Result<()> {
     let result = assemble_and_link(&[&a, &b], &Options::default())?;
 
     let expect = r#"OpCapability Kernel
-        OpName %1 "param"
-        OpName %2 "foo"
+        OpName %1 "foo"
+        OpName %2 "param"
         OpModuleProcessed "Linked by rspirv-linker"
-        OpDecorate %1 Restrict
         OpDecorate %3 Restrict
         OpDecorate %4 NonWritable
         %3 = OpDecorationGroup
         OpGroupDecorate %3 %4
+        OpDecorate %2 Restrict
         %5 = OpTypeVoid
         %6 = OpTypeInt 32 0
         %7 = OpTypePointer Function %6
@@ -498,8 +498,8 @@ fn names_and_decorations() -> Result<()> {
         %9 = OpFunction %5 None %8
         %4 = OpFunctionParameter %7
         OpFunctionEnd
-        %2 = OpFunction %5 None %8
-        %1 = OpFunctionParameter %7
+        %1 = OpFunction %5 None %8
+        %2 = OpFunctionParameter %7
         %10 = OpLabel
         OpReturn
         OpFunctionEnd"#;