Stream: git-wasmtime

Topic: wasmtime / PR #8581 cranelift: Stop consuming allocations

Wasmtime GitHub notifications bot (May 08 2024 at 01:51):

jameysharp opened PR #8581 from jameysharp:stop-consuming-allocs to bytecodealliance:main:

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.

Wasmtime GitHub notifications bot (May 08 2024 at 01:51):

jameysharp requested fitzgen for a review on PR #8581.

Wasmtime GitHub notifications bot (May 08 2024 at 01:51):

jameysharp requested wasmtime-compiler-reviewers for a review on PR #8581.

Wasmtime GitHub notifications bot (May 08 2024 at 01:58):

jameysharp updated PR #8581.

Wasmtime GitHub notifications bot (May 08 2024 at 02:33):

lpereira created PR review comment:

Minor, but could these assignments be removed too?

Wasmtime GitHub notifications bot (May 08 2024 at 02:33):

lpereira submitted PR review.

Wasmtime GitHub notifications bot (May 08 2024 at 02:35):

lpereira submitted PR review.

Wasmtime GitHub notifications bot (May 08 2024 at 02:35):

lpereira created PR review comment:

Is any of these functions that take an AllocationConsumer to pretty print actually using that parameter, or just some of them?

Wasmtime GitHub notifications bot (May 08 2024 at 06:54):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (May 08 2024 at 06:54):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (May 08 2024 at 06:54):

jameysharp created PR review 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; or let _ = 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 the tmp field and re-binds the name tmp to a copy of the field. Rust has syntax for doing this directly in the pattern match that bound the name tmp in the first place, which I'd prefer to use.
• let ri_hi = ri.hi; or let 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.

Wasmtime GitHub notifications bot (May 08 2024 at 06:54):

jameysharp created PR review 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.

Wasmtime GitHub notifications bot (May 08 2024 at 14:16):

abrown submitted PR review:

Thanks for pushing this forward!

Wasmtime GitHub notifications bot (May 08 2024 at 16:39):

jameysharp merged PR #8581.

Last updated: Dec 23 2024 at 12:05 UTC