Stream: git-wasmtime

Topic: wasmtime / PR #2622 Refactor x64::Inst to use OperandSize...


view this post on Zulip Wasmtime GitHub notifications bot (Feb 01 2021 at 02:43):

kaseyc edited PR #2622 from operand to main:

This is in preparation for refactoring all x64::Inst arms to use OperandSize.

Current uses of OperandSize fall into two categories:

1. XMM operations which require 32/64 bit operands
2. Immediates which only care about 64-bit or not.

Adds assertions to existing Inst constructors to check that they are passed valid sizes.
This change also removes the implicit widening of 1 and 2 byte values to 4 bytes. from_bytes() is only used by category 2, so removing this behavior will not change any visible behavior.

Overall this change should be a no-op.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 02 2021 at 04:56):

kaseyc edited PR #2622 from operand to main:

This is in preparation for refactoring all x64::Inst arms to use OperandSize.

Current uses of OperandSize fall into two categories:

1. XMM operations which require 32/64 bit operands
2. Immediates which only care about 64-bit or not.

Adds assertions to existing Inst constructors to check that they are passed valid sizes.
This change also removes the implicit widening of 1 and 2 byte values to 4 bytes. from_bytes() is only used by category 2, so removing this behavior will not change any visible behavior.

Overall this change should be a no-op.

Partially addresses #2586.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 02 2021 at 04:56):

kaseyc has marked PR #2622 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 00:33):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 00:33):

cfallin created PR Review Comment:

This is a nice helper for asserts! I think it might be good to make the name a bit more explicit: maybe is_one_of(sizes)?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 00:33):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 00:33):

cfallin created PR Review Comment:

We can do sizes.iter().any(|val| self == *val) here, I think.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 02:41):

kaseyc submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 02:41):

kaseyc created PR Review Comment:

This branch kind of jumped out at me. Both sides do similar things with OperandSize, but the if 0 case only widens I64 types (not B64 or R64).

I left it as is to keep this a pure refactor, but it struck me as odd.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 02:47):

kaseyc updated PR #2622 from operand to main:

This is in preparation for refactoring all x64::Inst arms to use OperandSize.

Current uses of OperandSize fall into two categories:

1. XMM operations which require 32/64 bit operands
2. Immediates which only care about 64-bit or not.

Adds assertions to existing Inst constructors to check that they are passed valid sizes.
This change also removes the implicit widening of 1 and 2 byte values to 4 bytes. from_bytes() is only used by category 2, so removing this behavior will not change any visible behavior.

Overall this change should be a no-op.

Partially addresses #2586.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 02:47):

kaseyc submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 02:47):

kaseyc created PR Review Comment:

That is a much better name, thank you.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 02:49):

kaseyc submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 02:50):

kaseyc created PR Review Comment:

Done, thanks.

I mostly write C++ these days and my Rust is, ah, rusty.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 02:50):

kaseyc updated PR #2622 from operand to main:

This is in preparation for refactoring all x64::Inst arms to use OperandSize.

Current uses of OperandSize fall into two categories:

1. XMM operations which require 32/64 bit operands
2. Immediates which only care about 64-bit or not.

Adds assertions to existing Inst constructors to check that they are passed valid sizes.
This change also removes the implicit widening of 1 and 2 byte values to 4 bytes. from_bytes() is only used by category 2, so removing this behavior will not change any visible behavior.

Overall this change should be a no-op.

Partially addresses #2586.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 08:49):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 08:49):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 08:49):

cfallin created PR Review Comment:

Hmm, yes, good catch -- I think that for consistency this should match the widening logic in the value != 0 case below. (It's harmless in practice as the 32-bit xor will zero the top 32 bits anyway, but we might as well be consistent.)

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 08:56):

kaseyc updated PR #2622 from operand to main:

This is in preparation for refactoring all x64::Inst arms to use OperandSize.

Current uses of OperandSize fall into two categories:

1. XMM operations which require 32/64 bit operands
2. Immediates which only care about 64-bit or not.

Adds assertions to existing Inst constructors to check that they are passed valid sizes.
This change also removes the implicit widening of 1 and 2 byte values to 4 bytes. from_bytes() is only used by category 2, so removing this behavior will not change any visible behavior.

Overall this change should be a no-op.

Partially addresses #2586.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 08:57):

kaseyc created PR Review Comment:

Updated.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 08:57):

kaseyc submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 03 2021 at 18:40):

abrown merged PR #2622.


Last updated: Dec 23 2024 at 12:05 UTC