Stream: git-wasmtime

Topic: wasmtime / PR #8826 Introduce the `cranelift-bitset` crat...

Wasmtime GitHub notifications bot (Jun 17 2024 at 20:31):

fitzgen opened PR #8826 from fitzgen:cranelift-bitset to bytecodealliance:main:

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Jun 17 2024 at 20:31):

fitzgen requested abrown for a review on PR #8826.

Wasmtime GitHub notifications bot (Jun 17 2024 at 20:31):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #8826.

Wasmtime GitHub notifications bot (Jun 17 2024 at 20:31):

fitzgen requested wasmtime-core-reviewers for a review on PR #8826.

Wasmtime GitHub notifications bot (Jun 17 2024 at 20:31):

fitzgen requested elliottt for a review on PR #8826.

Wasmtime GitHub notifications bot (Jun 17 2024 at 20:31):

fitzgen requested wasmtime-default-reviewers for a review on PR #8826.

Wasmtime GitHub notifications bot (Jun 17 2024 at 20:44):

fitzgen updated PR #8826.

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:06):

fitzgen updated PR #8826.

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:24):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:24):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:24):

alexcrichton created PR review comment:

Given the perf-sensitive nature of bitsets should this be debug_assert!?

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:24):

alexcrichton created PR review comment:

I think this might also need some parens to get the calculation you're shooting for

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:24):

alexcrichton created PR review comment:

I think this might need (...) to disambiguate?

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:24):

alexcrichton created PR review comment:

You can probably skip this due to the ? on last_mut()

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:24):

alexcrichton created PR review comment:

This is notably quite different from Vec::reserve where it's initializing the reserved space, is that desired here?

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:24):

alexcrichton created PR review comment:

(if it is desired I think this should probably get a different name)

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:24):

alexcrichton created PR review comment:

I think this needs to update self.len?

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:24):

alexcrichton created PR review comment:

Could this perhaps be combined with a match on last.pop()?

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:49):

fitzgen updated PR #8826.

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:53):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:53):

fitzgen created PR review comment:

I was thinking that originally, but figured we might as well do the more-correct thing by default and add unchecked versions later if profiling shows that these asserts actually matter.

I kind of expect LLVM to clean them all up, at least when they are called via CompoundBitSet methods, since it should be able to determine the ranges of values flowing into these methods based on the modulo operation in CompoundBitSet::word_and_bit.

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:53):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2024 at 21:53):

fitzgen created PR review comment:

Whoops!

Wasmtime GitHub notifications bot (Jun 17 2024 at 22:08):

fitzgen updated PR #8826.

Wasmtime GitHub notifications bot (Jun 17 2024 at 22:10):

fitzgen updated PR #8826.

Wasmtime GitHub notifications bot (Jun 17 2024 at 22:16):

fitzgen commented on PR #8826:

@alexcrichton this should be ready for a second round of review now.

Wasmtime GitHub notifications bot (Jun 18 2024 at 01:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 18 2024 at 01:48):

alexcrichton created PR review comment:

I might recommend renaming this method to resize or some variant thereof. The Vec::reserve method reserves space for extra capacity which isn't what this is doing so I'd be hesitant to create confusion between the two. Also Vec::reserve is mostly about preallocating uninitialized memory where this is specifically initializing the memory as well.

Wasmtime GitHub notifications bot (Jun 18 2024 at 01:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 18 2024 at 15:35):

fitzgen updated PR #8826.

Wasmtime GitHub notifications bot (Jun 18 2024 at 15:36):

fitzgen has enabled auto merge for PR #8826.

Wasmtime GitHub notifications bot (Jun 18 2024 at 15:57):

fitzgen merged PR #8826.

Last updated: Nov 22 2024 at 17:03 UTC