wasmtime / PR #8921 winch(arm64): and, or, xor, shifts · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #8921 winch(arm64): and, or, xor, shifts

Wasmtime GitHub notifications bot (Jul 09 2024 at 14:10):

evacchi edited PR #8921.

Wasmtime GitHub notifications bot (Jul 09 2024 at 14:17):

saulecabrera commented on PR #8921:

I'll take a look at this one.

Wasmtime GitHub notifications bot (Jul 09 2024 at 14:19):

alexcrichton requested saulecabrera for a review on PR #8921.

Wasmtime GitHub notifications bot (Jul 09 2024 at 22:26):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jul 09 2024 at 22:26):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jul 09 2024 at 22:26):

saulecabrera created PR review comment:

Most of the operations in this method are not machine dependent. The only difference, if I remember correctly is that in the case of x64, the number of bits to shift must be in the lower bits of the rcx register. This is not the case for aarch64 as far as I can tell. Perhaps we should move this code to visitor.rs / context.rs?

I'm imagining that we could:

Split the the masm methods into two: shift_ir and shift_rr

Move the common bits (of checking the stack top) to context.rs, similar to what we do for the binary operations? https://github.com/bytecodealliance/wasmtime/blob/main/winch/codegen/src/codegen/context.rs#L276

Wasmtime GitHub notifications bot (Jul 09 2024 at 22:26):

saulecabrera created PR review comment:

A conversion already exists between Winch's OperandSize and Cranelift's aarch64 OperandSize; I believe we could get drop this method and instead do something like
size.into().to_ty()

Wasmtime GitHub notifications bot (Jul 09 2024 at 22:26):

saulecabrera created PR review comment:

Given that this operation can fail, maybe we should avoid implementing Into/From and instead create a helper method in the Assembler itself, by extracting:

let shift_op: ALUOp = if kind == ShiftKind::Rotl {
            // Emulate Rotr with rm := neg(rm); with neg(x) := sub(zero, x).
            self.emit_alu_rrr(ALUOp::Sub, regs::zero(), rn, rn, size);
            ALUOp::RotR
        } else {
            kind.into()
        };

Wasmtime GitHub notifications bot (Jul 11 2024 at 20:12):

evacchi updated PR #8921.

Wasmtime GitHub notifications bot (Jul 11 2024 at 20:13):

evacchi submitted PR review.

Wasmtime GitHub notifications bot (Jul 11 2024 at 20:13):

evacchi created PR review comment:

refactored and moved to shift_kind_to_alu_op()

Wasmtime GitHub notifications bot (Jul 11 2024 at 20:13):

evacchi submitted PR review.

Wasmtime GitHub notifications bot (Jul 11 2024 at 20:13):

evacchi created PR review comment:

done!

Wasmtime GitHub notifications bot (Jul 11 2024 at 20:45):

evacchi submitted PR review.

Wasmtime GitHub notifications bot (Jul 11 2024 at 20:45):

evacchi created PR review comment:

I think I got it, will do

Wasmtime GitHub notifications bot (Jul 12 2024 at 22:02):

evacchi updated PR #8921.

Wasmtime GitHub notifications bot (Jul 12 2024 at 22:03):

evacchi submitted PR review.

Wasmtime GitHub notifications bot (Jul 12 2024 at 22:03):

evacchi created PR review comment:

I hope I got it right :grimacing:

Wasmtime GitHub notifications bot (Jul 12 2024 at 22:04):

evacchi submitted PR review.

Wasmtime GitHub notifications bot (Jul 12 2024 at 22:04):

evacchi created PR review comment:

I hope these are correct :grimacing:

Wasmtime GitHub notifications bot (Jul 12 2024 at 22:04):

evacchi requested saulecabrera for a review on PR #8921.

Wasmtime GitHub notifications bot (Jul 12 2024 at 22:05):

evacchi updated PR #8921.

Wasmtime GitHub notifications bot (Jul 16 2024 at 13:38):

saulecabrera updated PR #8921.

Wasmtime GitHub notifications bot (Jul 16 2024 at 13:54):

saulecabrera submitted PR review:

This looks good to me, thanks!

For visibility, here's a recap of an offline discussion between @evacchi and myself:

@evacchi is on PTO for the next couple of weeks and therefore suggested pushing any required changes on top of this PR

I pushed https://github.com/bytecodealliance/wasmtime/pull/8921/commits/e18681c3b716aa85b9361f20b9926fcb28b06bd1, which is a mechanical change only.

Given the recent changes I was thinking it might make sense to get a second review on this change.

@elliottt, would you be able to provide a second pass on this change?

Wasmtime GitHub notifications bot (Jul 16 2024 at 13:54):

saulecabrera requested elliottt for a review on PR #8921.

Wasmtime GitHub notifications bot (Jul 16 2024 at 13:54):

saulecabrera submitted PR review:

This looks good to me, thanks!

For visibility, here's a recap of an offline discussion between @evacchi and myself:

@evacchi is on PTO for the next couple of weeks and therefore suggested pushing any required changes on top of this PR in order to take it to the finish line.

I pushed https://github.com/bytecodealliance/wasmtime/pull/8921/commits/e18681c3b716aa85b9361f20b9926fcb28b06bd1, which is a mechanical change only.

Given the recent changes I was thinking it might make sense to get a second review on this change.

@elliottt, would you be able to provide a second pass on this change?

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

elliottt submitted PR review:

This looks good to me as well!

Wasmtime GitHub notifications bot (Jul 16 2024 at 22:32):

elliottt merged PR #8921.

Wasmtime GitHub notifications bot (Jul 17 2024 at 04:16):

evacchi commented on PR #8921:

Wonderful thanks for pushing this to the finish line!! :heart::heart:

Last updated: Dec 23 2024 at 12:05 UTC