Wasmtime GitHub notifications bot (Jan 27 2023 at 21:17): Wasmtime GitHub notifications bot (Jan 27 2023 at 21:17):fitzgen opened PR #5647 from mul-pow-2 to main:
Draft because this is hitting an ISLE codegen bug where the generated Rust code doesn't pass the borrow checker:
error[E0382]: use of moved value: `v26` --> | 577 | let mut v26 = v26; | ------- --- this reinitialization might get skipped | | | move occurs because `v26` has type `<C as opts::generated_code::Context>::inst_data_etor_iter`, which does not implement the `Copy` trait ... 598 | while let Some(v35) = v34.next(ctx) { | ----------------------------------- | | | inside of this loop | inside of this loop ... 606 | let v26 = v26; | --- value moved here, in previous iteration of loop ... 679 | let v26 = v26; | --- value moved here, in previous iteration of loop ... 723 | let v26 = v26; | ^^^ value used here after move For more information about this error, try `rustc --explain E0382`.
Wasmtime GitHub notifications bot (Jan 30 2023 at 20:54):fitzgen updated PR #5647 from mul-pow-2 to main.
Wasmtime GitHub notifications bot (Jan 30 2023 at 20:55):fitzgen requested jameysharp for a review on PR #5647.
Wasmtime GitHub notifications bot (Jan 30 2023 at 20:55):fitzgen has marked PR #5647 as ready for review.
Wasmtime GitHub notifications bot (Jan 30 2023 at 21:30):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Jan 30 2023 at 21:30):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Jan 30 2023 at 21:30):jameysharp created PR review comment:
I'd be inclined to return
x.trailing_zeros()here instead of justx, and remove theu64_log2extractor. When you're asking whether something's a power of two, it's natural to wonder which power of two it is. On the flip side, usingtrailing_zerosto implementilog2is safe _iff_x.is_power_of_two()is true. So I think it makes the most sense to couple these two queries together pretty tightly. If somebody really just wants the original value after checking that it's a power of two, they can use at-patterns to bind the input instead of the result.I also wonder, should this extractor take
u64as input instead ofImm64? We have extractors for converting in either direction so I guess it doesn't matter too much which way we pick here.
Wasmtime GitHub notifications bot (Jan 30 2023 at 21:30):jameysharp created PR review comment:
Is this debug log message generally useful, or a leftover?
Wasmtime GitHub notifications bot (Jan 30 2023 at 23:43):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jan 30 2023 at 23:43):fitzgen created PR review comment:
It is generally useful. So hard to get the input that the patterns are being filechecked against, now we can always see it via
RUST_LOG=debug.
Wasmtime GitHub notifications bot (Jan 30 2023 at 23:45):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jan 30 2023 at 23:45):fitzgen created PR review comment:
Yeah, I like that!
I also wonder, should this extractor take u64 as input instead of Imm64? We have extractors for converting in either direction so I guess it doesn't matter too much which way we pick here.
Its just more wordy since we don't have automatic conversions between the two.
Wasmtime GitHub notifications bot (Jan 30 2023 at 23:46):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jan 30 2023 at 23:46):fitzgen created PR review comment:
(And I don't think we necessarily should since
Imm64is "signed" but also it kind of depends on how the value ends up being used.)
Wasmtime GitHub notifications bot (Jan 31 2023 at 17:09):fitzgen updated PR #5647 from mul-pow-2 to main.
Wasmtime GitHub notifications bot (Jan 31 2023 at 17:10):fitzgen has enabled auto merge for PR #5647.
Wasmtime GitHub notifications bot (Jan 31 2023 at 18:04):fitzgen merged PR #5647.
Last updated: Dec 23 2024 at 13:07 UTC