Stream: git-wasmtime

Topic: wasmtime / PR #2148 Fix put_input_in_reg

Wasmtime GitHub notifications bot (Aug 19 2020 at 17:07):

bjorn3 opened PR #2148 from aarch64_fix_put_input_in_rsa to main:

Fixes #2147

Wasmtime GitHub notifications bot (Aug 19 2020 at 17:38):

bjorn3 updated PR #2148 from aarch64_fix_put_input_in_rsa to main:

Fixes #2147

Wasmtime GitHub notifications bot (Aug 19 2020 at 18:16):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 19 2020 at 18:16):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 19 2020 at 18:16):

cfallin created PR Review Comment:

Hmm, this isn't quite right: we should be able to rely directly on the sxtb extend-mode below; the separate instruction is redundant.

Wasmtime GitHub notifications bot (Aug 19 2020 at 18:16):

cfallin created PR Review Comment:

So I think the fix is much simpler, actually:

The issue here is that we catch the "output smaller than 32 bits" case before we catch the "op is a uextend/sextend" case, so we end up redundantly generating an extend instruction and then using the extend mode as well.

The solution to the smaller-than-32-bits case is to realize that we actually can "over-extend" to a 32- or 64-bit result and this is still correct. E.g., a caller asking for an i8 arg to be zero-extended will be satisfied by a value that is extended to 32 or 64 bits.

So I think we can simply remove this whole if-statement (pre-diff lines 354 to 383), and the out_bits assert below on line 387, and things should work. This should get rid of the redundant extend instructions you had to add to tests below.

Wasmtime GitHub notifications bot (Aug 19 2020 at 19:19):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 19 2020 at 19:19):

bjorn3 created PR Review Comment:

When you have a non-uextend/sextend instruction outputing a small integer and narrow_mode is not None, you still need to perform the extension I think. By removing the smaller-than-32-bits case, that would always result in a ResultRSE::Reg instead of a ResultRSE::RegExtend.

Wasmtime GitHub notifications bot (Aug 20 2020 at 00:39):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 20 2020 at 00:39):

cfallin created PR Review Comment:

Oh, that's true, my suggestion would be suboptimal in that case -- the narrow_mode would make it all the way to put_input_in_reg() and would force an explicit Inst::Extend.

So perhaps the best heuristic is to swap the extend-from-narrower-than-32-bits case (this if-block) and the explicit uextend/sextend match (line 386), so that we always eat the extend instruction (this handles the original case you raised in #2147); then make another attempt to use an extend-op to handle the NarrowValueMode itself; then fall back to reg/shift cases. Thoughts?

Wasmtime GitHub notifications bot (Aug 20 2020 at 08:17):

bjorn3 updated PR #2148 from aarch64_fix_put_input_in_rsa to main:

Fixes #2147

Wasmtime GitHub notifications bot (Aug 20 2020 at 08:18):

bjorn3 created PR Review Comment:

I dropped these, as there is no i1 type and extension of b1 should use bextend and not uextend or sextend.

Wasmtime GitHub notifications bot (Aug 20 2020 at 08:18):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 20 2020 at 08:20):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Aug 20 2020 at 08:20):

bjorn3 created PR Review Comment:

Done. Also checked that both the uextend/sextend and the narrow_mode have an equal sign. The diff on the other tests is gone.

Wasmtime GitHub notifications bot (Aug 20 2020 at 18:41):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 20 2020 at 18:41):

cfallin merged PR #2148.

Last updated: Dec 23 2024 at 13:07 UTC