Merge pull request #1078 from CohenArthur/remove-clone-on-nop-filling

Remove clone() when removing extra stack operations

Author: bjorn3

Cargo.lock

@@ -23,6 +23,7 @@ byteorder = "1.2.7"
 indexmap = "1.0.2"
 cfg-if = "0.1.10"
 libloading = { version = "0.6.0", optional = true }
+hashbrown = "0.8.1"
 
 # Uncomment to use local checkout of cranelift
 #[patch."https://github.com/bytecodealliance/wasmtime/"]

Cargo.toml

@@ -13,12 +13,15 @@ use std::collections::BTreeMap;
 use std::fmt;
 use std::ops::Not;
 
-use rustc_data_structures::fx::FxHashSet;
+use rustc_data_structures::fx::{FxHashSet, FxHasher};
 
 use cranelift_codegen::cursor::{Cursor, FuncCursor};
 use cranelift_codegen::ir::{InstructionData, Opcode, ValueDef};
 use cranelift_codegen::ir::immediates::Offset32;
 
+use hashbrown::HashSet;
+use std::hash::BuildHasherDefault;
+
 use crate::prelude::*;
 
 /// Workaround for `StackSlot` not implementing `Ord`.
@@ -45,9 +48,9 @@ impl Ord for OrdStackSlot {
 
 #[derive(Debug, Default)]
 struct StackSlotUsage {
-    stack_addr: FxHashSet<Inst>,
-    stack_load: FxHashSet<Inst>,
-    stack_store: FxHashSet<Inst>,
+    stack_addr: HashSet<Inst, BuildHasherDefault<FxHasher>>,
+    stack_load: HashSet<Inst, BuildHasherDefault<FxHasher>>,
+    stack_store: HashSet<Inst, BuildHasherDefault<FxHasher>>,
 }
 
 impl StackSlotUsage {
@@ -79,16 +82,14 @@ impl StackSlotUsage {
         }).collect::<Vec<Inst>>()
     }
 
-    fn remove_unused_stack_addr(&mut self, func: &mut Function, inst: Inst) {
+    fn remove_unused_stack_addr(func: &mut Function, inst: Inst) {
         func.dfg.detach_results(inst);
         func.dfg.replace(inst).nop();
-        self.stack_addr.remove(&inst);
     }
 
-    fn remove_unused_load(&mut self, func: &mut Function, load: Inst) {
+    fn remove_unused_load(func: &mut Function, load: Inst) {
         func.dfg.detach_results(load);
         func.dfg.replace(load).nop();
-        self.stack_load.remove(&load);
     }
 
     fn remove_dead_store(&mut self, func: &mut Function, store: Inst) {
@@ -314,19 +315,21 @@ fn remove_unused_stack_addr_and_stack_load(opt_ctx: &mut OptimizeContext<'_>) {
     }
 
     // Replace all unused stack_addr and stack_load instructions with nop.
-    for stack_slot_users in opt_ctx.stack_slot_usage_map.values_mut() {
-        // FIXME remove clone
-        for &inst in stack_slot_users.stack_addr.clone().iter() {
-            if stack_addr_load_insts_users.get(&inst).map(|users| users.is_empty()).unwrap_or(true) {
-                stack_slot_users.remove_unused_stack_addr(&mut opt_ctx.ctx.func, inst);
-            }
-        }
+    let mut func = &mut opt_ctx.ctx.func;
 
-        for &inst in stack_slot_users.stack_load.clone().iter() {
-            if stack_addr_load_insts_users.get(&inst).map(|users| users.is_empty()).unwrap_or(true) {
-                stack_slot_users.remove_unused_load(&mut opt_ctx.ctx.func, inst);
-            }
-        }
+    // drain_filter() on hashbrown::HashSet drains the items that do *not* match the
+    // predicate. Once hashbrown gets updated to match the behaviour of std::drain_filter
+    // (0.8.2), the predicate will have to be reversed
+    for stack_slot_users in opt_ctx.stack_slot_usage_map.values_mut() {
+        stack_slot_users
+            .stack_addr
+            .drain_filter(|inst| !(stack_addr_load_insts_users.get(inst).map(|users| users.is_empty()).unwrap_or(true)))
+            .for_each(|inst| StackSlotUsage::remove_unused_stack_addr(&mut func, inst));
+
+        stack_slot_users
+            .stack_load
+            .drain_filter(|inst| !(stack_addr_load_insts_users.get(inst).map(|users| users.is_empty()).unwrap_or(true)))
+            .for_each(|inst| StackSlotUsage::remove_unused_load(&mut func, inst));
     }
 }