Wasmtime GitHub notifications bot (Feb 01 2021 at 02:43): 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.
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.
Wasmtime GitHub notifications bot (Feb 02 2021 at 04:56):kaseyc has marked PR #2622 as ready for review.
Wasmtime GitHub notifications bot (Feb 03 2021 at 00:33):cfallin submitted PR Review.
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)?
Wasmtime GitHub notifications bot (Feb 03 2021 at 00:33):cfallin submitted PR Review.
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.
Wasmtime GitHub notifications bot (Feb 03 2021 at 02:41):kaseyc submitted PR Review.
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.
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.
Wasmtime GitHub notifications bot (Feb 03 2021 at 02:47):kaseyc submitted PR Review.
Wasmtime GitHub notifications bot (Feb 03 2021 at 02:47):kaseyc created PR Review Comment:
That is a much better name, thank you.
Wasmtime GitHub notifications bot (Feb 03 2021 at 02:49):kaseyc submitted PR Review.
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.
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.
Wasmtime GitHub notifications bot (Feb 03 2021 at 08:49):cfallin submitted PR Review.
Wasmtime GitHub notifications bot (Feb 03 2021 at 08:49):cfallin submitted PR Review.
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 != 0case below. (It's harmless in practice as the 32-bitxorwill zero the top 32 bits anyway, but we might as well be consistent.)
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.
Wasmtime GitHub notifications bot (Feb 03 2021 at 08:57):kaseyc created PR Review Comment:
Updated.
Wasmtime GitHub notifications bot (Feb 03 2021 at 08:57):kaseyc submitted PR Review.
Wasmtime GitHub notifications bot (Feb 03 2021 at 18:40):abrown merged PR #2622.
Last updated: Nov 22 2024 at 16:03 UTC