cranelift: Stop consuming allocations #8581
Conversation
The `next` and `next_writable` methods on `AllocationConsumer` are identity functions now, so replace each call with its argument and then clean up the resulting mess. Most of this commit was generated with these commands: - `git grep -lF allocs.next cranelift/codegen/src/isa/ | xargs sed -i 's/allocs\.next\(_writable\)\?//'` - `cargo fix -p cranelift-codegen --features all-arch --allow-dirty --allow-staged` - `git diff --name-only HEAD | xargs sed -i '/let \([^ ]*\) = \1;/d'` - `cargo fmt` I used sed to delete `allocs.next` but left the following parentheses alone (since matching balanced parentheses is not a regular language). Then I used `cargo fix` to remove those parentheses and also add underscores to newly-unused `AllocationConsumer` arguments. Next I used sed again to delete trivial assignments like `let x = x`, leaving more complicated cases as future work to clean up, and finally `cargo fmt` to delete blank lines and braces that are no longer necessary. Last, I deleted the newly-unused definitions of `next` and `next_writable` themselves.
jameysharp
force-pushed
the
stop-consuming-allocs
branch
from
c81d65a to
06acb15
Compare
| @@ -2121,7 +2114,7 @@ impl Inst { | |||
| } | |||
| &Inst::FpuCMov64 { rd, cond, ri, rm } => { | |||
| let (rd, rd_fpr) = pretty_print_fpr(rd.to_reg(), allocs); | |||
| let _ri = allocs.next(ri); | |||
| let _ri = ri; | |||
There was a problem hiding this comment.
Minor, but could these assignments be removed too?
There was a problem hiding this comment.
Yes, there are several kinds of assignments that should still be removed, but they're more difficult to do mechanically so I wanted to save them for a separate PR.
let _ri = ri;orlet _ = u.vreg;has the side effect of suppressing warnings that the value on the right-hand side is not used. This is not an idiomatic way to do it, but simply deleting the assignment will introduce warnings, which breaks CI unless we do something about them.let tmp = *tmp;dereferences a borrow of thetmpfield and re-binds the nametmpto a copy of the field. Rust has syntax for doing this directly in the pattern match that bound the nametmpin the first place, which I'd prefer to use.let ri_hi = ri.hi;orlet r = rn;could be deleted as long as all the uses of the left-hand side are replaced by the right-hand side.
The changes in this PR are only the ones I could script with sed and the other tools I was using, and I want to stop at this point so this review is pretty mechanical too.
| @@ -176,8 +176,7 @@ fn show_reg(reg: Reg) -> String { | |||
| } | |||
| } | |||
|
|
|||
| pub fn pretty_print_reg(reg: Reg, allocs: &mut AllocationConsumer) -> String { | |||
| let reg = allocs.next(reg); | |||
| pub fn pretty_print_reg(reg: Reg, _allocs: &mut AllocationConsumer) -> String { | |||
There was a problem hiding this comment.
Is any of these functions that take an AllocationConsumer to pretty print actually using that parameter, or just some of them?
There was a problem hiding this comment.
Right, none of the AllocationConsumer arguments are actually necessary, after #8524 landed. I'm deleting all the newly dead code in several steps to make review easier, so this PR deletes all remaining calls to AllocationConsumer methods.
There are still some functions where the Rust compiler thinks the AllocationConsumer argument is used, but that's because it's passed to other functions that don't actually need it either. I intend to delete all those arguments, and all mentions of the AllocationConsumer type, in another PR after this one.
github-actions
bot
added
cranelift
Part of the ongoing saga of bytecodealliance#8524, bytecodealliance#8566, bytecodealliance#8581, and bytecodealliance#8592
The
nextandnext_writablemethods onAllocationConsumerare identity functions now, so replace each call with its argument and then clean up the resulting mess.Most of this commit was generated with these commands:
git grep -lF allocs.next cranelift/codegen/src/isa/ | xargs sed -i 's/allocs\.next\(_writable\)\?//'cargo fix -p cranelift-codegen --features all-arch --allow-dirty --allow-stagedgit diff --name-only HEAD | xargs sed -i '/let \([^ ]*\) = \1;/d'cargo fmtI used sed to delete
allocs.nextbut left the following parentheses alone (since matching balanced parentheses is not a regular language). Then I usedcargo fixto remove those parentheses and also add underscores to newly-unusedAllocationConsumerarguments. Next I used sed again to delete trivial assignments likelet x = x, leaving more complicated cases as future work to clean up, and finallycargo fmtto delete blank lines and braces that are no longer necessary.Last, I deleted the newly-unused definitions of
nextandnext_writablethemselves.