Remove clone() when removing extra stack operations #1078
Conversation
|
I noticed this creates an infinite loop. Sorry about that, I couldn't run all the tests on my computer. I'm reworking the PR |
src/optimize/stack2reg.rs
Outdated
| @@ -315,7 +315,7 @@ 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 | |||
| // FIXME: Remove clone | |||
| for &inst in stack_slot_users.stack_addr.clone().iter() { | |||
There was a problem hiding this comment.
Maybe use drain_filter with |inst| stack_addr_load_insts_users.get(inst).map(|users| users.is_empty()).unwrap_or(true) as argument and then remove the self.stack_addr.remove(inst) from remove_unused_stack_addr and remove the self param of it?
There was a problem hiding this comment.
That was a very good idea and it set me on the correct path. Thanks
src/optimize/stack2reg.rs
Outdated
| .iter() | ||
| .filter(|inst| stack_addr_load_insts_users.get(inst).map(|users| users.is_empty()).unwrap_or(true)) |
There was a problem hiding this comment.
This should use drain_filter to also remove the insts that match.
There was a problem hiding this comment.
Yes sorry, I forgot. It's a bit hard to run tests on my laptop.
There was a problem hiding this comment.
Should be fixed in 5c8c75b. Also drain_filter() isn't directly available, so I'm using drain().filter()
There was a problem hiding this comment.
drain().filter() drains everything, while drain_filter() only drains some. What do you mean with "isn't directly available"? If you mean behing a feature-gate, feel free to add it. cg_clif is forever fixed to internal api's anyway.
There was a problem hiding this comment.
What I mean is that drain_filter() isn't implemented for HashSet. See this. It's implemented in the hashbrown crate, but from my understanding it hasn't landed in the standard library yet.
There was a problem hiding this comment.
Ah, I thought it was a Vec. In that case I don't know how to do it.
There was a problem hiding this comment.
I'm going to look into it. Either we wait for it to land, or we implement our own as a small patch for now.
There was a problem hiding this comment.
I don't think it is possible to implement drain_filter without access to internal fields of the HashSet implementation. Maybe directly use hashbrown::HashSet? (With rustc_data_structures::fx::FxHasher as hasher)
There was a problem hiding this comment.
Okay, done. However, the behavior of drain_filter() in hashbrown hashset is gonna be reverted. For now, I simply had to revert the predicate, but once hashbrown gets updated (0.8.2) we'll have to reverse it. So far it's only a comment in the code, and I specified the hashbrown version as "0.8.1"
There was a problem hiding this comment.
Here is the changelog explaining the current behavior of drain_filter()
Replace the double
clone()inremove_unused_stack_addr_and_stack_load()with awhile letexpression.