Stream: git-wasmtime

Topic: wasmtime / PR #2489 [machinst x64]: implement load*_zero ...

Wasmtime GitHub notifications bot (Dec 08 2020 at 16:51):

abrown requested cfallin and julian-seward1 for a review on PR #2489.

Wasmtime GitHub notifications bot (Dec 08 2020 at 16:51):

abrown opened PR #2489 from load-zero to main:

Like #2480, this should not be merged until #2432 is resolved and x64 Wasm SIMD spec tests are turned back on. The simd_load_zero.wast test is now in-tree since #2481 but will have to be enabled in the build.rs file.

Wasmtime GitHub notifications bot (Dec 08 2020 at 16:51):

abrown requested cfallin and julian-seward1 for a review on PR #2489.

Wasmtime GitHub notifications bot (Dec 08 2020 at 16:52):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Dec 08 2020 at 16:52):

bjorn3 created PR Review Comment:

nit: missing trailing newline

Wasmtime GitHub notifications bot (Jan 05 2021 at 00:09):

abrown updated PR #2489 from load-zero to main:

Like #2480, this should not be merged until #2432 is resolved and x64 Wasm SIMD spec tests are turned back on. The simd_load_zero.wast test is now in-tree since #2481 but will have to be enabled in the build.rs file.

Wasmtime GitHub notifications bot (Jan 05 2021 at 00:11):

abrown updated PR #2489 from load-zero to main:

Like #2480, this should not be merged until #2432 is resolved and x64 Wasm SIMD spec tests are turned back on. The simd_load_zero.wast test is now in-tree since #2481 but will have to be enabled in the build.rs file.

Wasmtime GitHub notifications bot (Jan 05 2021 at 00:13):

abrown updated PR #2489 from load-zero to main:

Like #2480, this should not be merged until #2432 is resolved and x64 Wasm SIMD spec tests are turned back on. The simd_load_zero.wast test is now in-tree since #2481 but will have to be enabled in the build.rs file.

Wasmtime GitHub notifications bot (Jan 05 2021 at 00:35):

abrown has marked PR #2489 as ready for review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 01:06):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 01:06):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 01:06):

cfallin created PR Review Comment:

Did you work out what the issue was with the earlier version of this test that could not coalesce?

Wasmtime GitHub notifications bot (Jan 05 2021 at 17:38):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 17:38):

abrown created PR Review Comment:

I debug-ed to here and realized that indeed the insertion of the block was preventing input_to_reg_mem from seeing the input as mergeable. I realized I didn't understand how the coloring is working, but my hazy mental model makes me think that the logic should not be "allow sinking if the color is +1" but rather "allow sinking if the color is 0 or +1". I mean, I haven't looked too closely at the logic, but from the comments in src/machinst/lower.rs, I would expect the following code to coalesce, but it does not:

function %load32_zero(i64) -> i32x4 {
block0(v0: i64):
    v1 = load.i32 v0
    v2 = scalar_to_vector.i32x4 v1
    jump block1
 block1:
    return v2
}

In any case, the optimization here is a separate issue than implementing the load*_zero instruction itself.

Wasmtime GitHub notifications bot (Jan 05 2021 at 17:48):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 17:48):

cfallin created PR Review Comment:

So the reason for the +1 correction is that the test is comparing the input color of the possibly-merged load to the color at the merging point; and because loads are effectful (in this model), the output color of the load is input+1. So the test is really just "output color of load is current color". That said, it's possible that the new-block increment is happening at the wrong place such that we disallow this case -- can debug further in a bit...

Wasmtime GitHub notifications bot (Jan 06 2021 at 01:57):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jan 06 2021 at 01:57):

cfallin created PR Review Comment:

(To clarify the above: I'm fine with this PR merging as-is, but please feel free to create a tracking issue for this coalescing case if still unsolved!)

Wasmtime GitHub notifications bot (Jan 09 2021 at 00:21):

abrown created PR Review Comment:

Ok, opened #2562 to track this; will merge as-is to keep things moving along.

Wasmtime GitHub notifications bot (Jan 09 2021 at 00:21):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jan 09 2021 at 00:21):

abrown merged PR #2489.

Last updated: Nov 22 2024 at 17:03 UTC