Skip to content

Move the default block to the front of the underlying jump table storage#5770

Merged
elliottt merged 1 commit intobytecodealliance:mainfrom
elliottt:trevor/default-block-first
Feb 13, 2023
Merged

Move the default block to the front of the underlying jump table storage#5770
elliottt merged 1 commit intobytecodealliance:mainfrom
elliottt:trevor/default-block-first

Conversation

@elliottt
Copy link
Member

The new api on JumpTableData makese it easy to keep the default label first, and that shrinks the diff in #5731 a bit.

@elliottt elliottt requested a review from jameysharp February 13, 2023 19:52
Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

Looks right to me!

Comment on lines +31 to +34
pub fn new(def: Block, table: &[Block]) -> Self {
Self {
table: std::iter::once(def).chain(table.iter().copied()).collect(),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

While reviewing this I considered two alternative implementations but decided I like the one you've chosen best.

One alternative is to continue to accept a Vec and use table.insert(0, def), which implicitly does a memmove. Some callers already have a Vec allocated and it's a little annoying to have to allocate a second one to copy into, only for the caller to drop theirs immediately afterward.

The other alternative is to take table: impl IntoIterator<Item = Block>. That's somewhat appealing for cranelift/wasm/src/code_translator.rs which I think could then avoid allocating a temporary Vec. It also works fine for all the tests since arrays implement the right IntoIterator. Passing in a Vec by-value works too, although like the current approach that still allocates a new Vec and copies into it. However, I think this is a little more magic than necessary. Passing in a slice is fine.

@elliottt elliottt enabled auto-merge (squash) February 13, 2023 20:12
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:wasm labels Feb 13, 2023
@elliottt elliottt merged commit 19f337e into bytecodealliance:main Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:wasm cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants