Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove clone() when removing extra stack operations #1078

Merged
merged 6 commits into from
Aug 23, 2020
Merged

Remove clone() when removing extra stack operations #1078

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c62f433
remove_unused_stack_addr_and_stack_load: Remove clone()
CohenArthur Aug 23, 2020
80ee042
stack2reg: Re-add clone() to stop CI
CohenArthur Aug 23, 2020
b0ea85f
stack2reg: Remove self in remove_unused_stack_addr and()
CohenArthur Aug 23, 2020
eb6df58
stack2reg: Remove clone() using filters
CohenArthur Aug 23, 2020
5c8c75b
stack2reg: Drain instead of only iterating
CohenArthur Aug 23, 2020
cb38689
stack2reg: Switch to hashbrown::HashSet
CohenArthur Aug 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 14 additions & 16 deletions src/optimize/stack2reg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,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) {
Expand Down Expand Up @@ -314,19 +312,19 @@ fn remove_unused_stack_addr_and_stack_load(opt_ctx: &mut OptimizeContext<'_>) {
}

// Replace all unused stack_addr and stack_load instructions with nop.
let mut func = &mut opt_ctx.ctx.func;
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);
}
}

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);
}
}
stack_slot_users
.stack_addr
.iter()
.filter(|inst| stack_addr_load_insts_users.get(inst).map(|users| users.is_empty()).unwrap_or(true))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use drain_filter to also remove the insts that match.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes sorry, I forgot. It's a bit hard to run tests on my laptop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed in 5c8c75b. Also drain_filter() isn't directly available, so I'm using drain().filter()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@CohenArthur CohenArthur Aug 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought it was a Vec. In that case I don't know how to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

@CohenArthur CohenArthur Aug 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the changelog explaining the current behavior of drain_filter()

.for_each(|inst| StackSlotUsage::remove_unused_stack_addr(&mut func, *inst));

stack_slot_users
.stack_load
.iter()
.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));
}
}

Expand Down