Stream: git-wasmtime

Topic: wasmtime / PR #6979 Cranelift: Improve codegen of store_i...

Wasmtime GitHub notifications bot (Sep 08 2023 at 05:19):

mchesser opened PR #6979 from mchesser:x64-store-imm to bytecodealliance:main:

Improve code generation of store_imm on x64 by adding a new rule that directly handles stores of small immediates.

Currently storing to constant to memory is achieved by first copying the constant into a temporary register, then performing a store of the register value to memory. For example:

    v1 = load.i64 v0
    v2 = iconst.i8 1
    store v2, v1

Is currently compiled to:

    MOV RCX,qword ptr [RDI]
    MOV EDX,0x1
    MOV byte ptr [RCX],DL

On x86 constants can be stored directly to memory, e.g., after this patch, the constant is inlined into the store instruction removing the extra temporary register.

    MOV RCX,qword ptr [RDI]
    MOV byte ptr [RCX],0x1

The overall performance impact is probably very minimal, but it could help with frontend/decoder pressure in cases where this pattern is common.

(Draft PR since this was partially an exercise in understanding ISLE so I could be missing something here).

Wasmtime GitHub notifications bot (Sep 09 2023 at 17:34):

alexcrichton submitted PR review:

Thanks for this!

There's a few tests which need to be updated which can be done with CRANELIFT_TEST_BLESS=1 cargo run test filetests from the cranelift directory. Otherwise though could you additionally add some *.clif tests in the cranelift/filetests/filetests/isa/x64/ directory specifically for these lowering rules? I think it'd be good to showcase a range of constants being stored to ensure that they call get codegen'd correctly.

Wasmtime GitHub notifications bot (Sep 09 2023 at 17:34):

alexcrichton submitted PR review:

Thanks for this!

There's a few tests which need to be updated which can be done with CRANELIFT_TEST_BLESS=1 cargo run test filetests from the cranelift directory. Otherwise though could you additionally add some *.clif tests in the cranelift/filetests/filetests/isa/x64/ directory specifically for these lowering rules? I think it'd be good to showcase a range of constants being stored to ensure that they call get codegen'd correctly.

Wasmtime GitHub notifications bot (Sep 09 2023 at 17:34):

alexcrichton created PR review comment:

If you're up for it I think it might be good to update the MovImmM variant itself. Looking at emit.rs it will panic if the low 32-bits of the value here don't sign-extend into the full 64-bits, so the MovImmM should I think store i32 instead of i64. With that you could add extra matching here that value fits within a 32-bit signed integer.

Wasmtime GitHub notifications bot (Sep 10 2023 at 03:11):

mchesser updated PR #6979.

Wasmtime GitHub notifications bot (Sep 10 2023 at 03:32):

mchesser updated PR #6979.

Wasmtime GitHub notifications bot (Sep 10 2023 at 04:00):

mchesser has marked PR #6979 as ready for review.

Wasmtime GitHub notifications bot (Sep 10 2023 at 04:00):

mchesser requested elliottt for a review on PR #6979.

Wasmtime GitHub notifications bot (Sep 10 2023 at 04:00):

mchesser requested wasmtime-compiler-reviewers for a review on PR #6979.

Wasmtime GitHub notifications bot (Sep 11 2023 at 13:53):

alexcrichton submitted PR review:

Thanks again for this!

Wasmtime GitHub notifications bot (Sep 11 2023 at 13:53):

alexcrichton submitted PR review:

Thanks again for this!

Wasmtime GitHub notifications bot (Sep 11 2023 at 13:53):

alexcrichton created PR review comment:

cc @saulecabrera, this is making the preexisting panic more explicit but it's one where I think the Err case will want to be handled via a move-to-register then store instruction.

(in a future PR, no need to do anything here @mchesser)

Wasmtime GitHub notifications bot (Sep 11 2023 at 14:42):

alexcrichton merged PR #6979.

Last updated: Dec 23 2024 at 12:05 UTC