wasmtime / PR #2240 [machinst x64]: calculate extension m... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #2240 [machinst x64]: calculate extension m...

Wasmtime GitHub notifications bot (Sep 28 2020 at 20:00):

abrown opened PR #2240 from refactor-ext-mode to main:

In previous PRs I noticed that the logic for calculating the extension modes is duplicated throughout the x64 module. This change moves the logic to a single place ExtMode::calculate and removes the duplication by calling this function (we can inline this if there are performance concerns). The more pressing question is whether the more expansive definition used in ExtMode::calculate is correct in all of the places it is used; some of these only handled, e.g. {8 -> 64, 16 -> 64, 32 -> 64}, and it is unclear to me whether the other extension modes should be matched here.

Wasmtime GitHub notifications bot (Sep 28 2020 at 20:01):

abrown updated PR #2240 from refactor-ext-mode to main:

In previous PRs I noticed that the logic for calculating the extension modes is duplicated throughout the x64 module. This change moves the logic to a single place ExtMode::calculate and removes the duplication by calling this function (we can inline this if there are performance concerns). The more pressing question is whether the more expansive definition used in ExtMode::calculate is correct in all of the places it is used; some of these only handled, e.g. {8 -> 64, 16 -> 64, 32 -> 64}, and it is unclear to me whether the other extension modes should be matched here.

Wasmtime GitHub notifications bot (Sep 28 2020 at 20:06):

abrown updated PR #2240 from refactor-ext-mode to main:

In previous PRs I noticed that the logic for calculating the extension modes is duplicated throughout the x64 module. This change moves the logic to a single place ExtMode::calculate and removes the duplication by calling this function (we can inline this if there are performance concerns). The more pressing question is whether the more expansive definition used in ExtMode::calculate is correct in all of the places it is used; some of these only handled, e.g. {8 -> 64, 16 -> 64, 32 -> 64}, and it is unclear to me whether the other extension modes should be matched here.

Wasmtime GitHub notifications bot (Sep 28 2020 at 20:16):

abrown updated PR #2240 from refactor-ext-mode to main:

In previous PRs I noticed that the logic for calculating the extension modes is duplicated throughout the x64 module. This change moves the logic to a single place ExtMode::calculate and removes the duplication by calling this function (we can inline this if there are performance concerns). The more pressing question is whether the more expansive definition used in ExtMode::calculate is correct in all of the places it is used; some of these only handled, e.g. {8 -> 64, 16 -> 64, 32 -> 64}, and it is unclear to me whether the other extension modes should be matched here.

Wasmtime GitHub notifications bot (Sep 29 2020 at 12:45):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Sep 29 2020 at 12:45):

bnjbvr created PR Review Comment:

Shouldn't this be src_ty.bits() < dst_ty.bits() || ext_mode.is_none()?

Wasmtime GitHub notifications bot (Sep 29 2020 at 12:45):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Sep 29 2020 at 12:45):

bnjbvr created PR Review Comment:

calculate made me think that there would be some complicated calculation here happening here; how about using a plain new here? (or infer, or for which would read nicely ExtMode::for(32, 64))

Wasmtime GitHub notifications bot (Sep 29 2020 at 15:46):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Sep 29 2020 at 15:46):

abrown created PR Review Comment:

Yeah, I like for or even from... ugh, choosing names. I'll go with for.

Wasmtime GitHub notifications bot (Sep 29 2020 at 16:17):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Sep 29 2020 at 16:17):

bjorn3 created PR Review Comment:

for is a keyword, so you can't use it as identifier.

Wasmtime GitHub notifications bot (Sep 29 2020 at 16:57):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Sep 29 2020 at 16:57):

abrown created PR Review Comment:

Then I guess new or from?

Wasmtime GitHub notifications bot (Sep 29 2020 at 18:12):

abrown created PR Review Comment:

Yeah, looking at it again I think it should actually be: (src_ty.bits() < dst_ty.bits() && ext_mode.is_some()) || ext_mode.is_none(). Either we have the right size bits and we get a proper Some(ExtMode) or we don't and get a None--anything else should be an error.

Wasmtime GitHub notifications bot (Sep 29 2020 at 18:12):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Sep 29 2020 at 18:13):

abrown updated PR #2240 from refactor-ext-mode to main:

In previous PRs I noticed that the logic for calculating the extension modes is duplicated throughout the x64 module. This change moves the logic to a single place ExtMode::calculate and removes the duplication by calling this function (we can inline this if there are performance concerns). The more pressing question is whether the more expansive definition used in ExtMode::calculate is correct in all of the places it is used; some of these only handled, e.g. {8 -> 64, 16 -> 64, 32 -> 64}, and it is unclear to me whether the other extension modes should be matched here.

Wasmtime GitHub notifications bot (Sep 29 2020 at 18:14):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Sep 29 2020 at 18:14):

abrown created PR Review Comment:

Went with new.

Wasmtime GitHub notifications bot (Sep 29 2020 at 21:48):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Sep 29 2020 at 21:48):

abrown created PR Review Comment:

Fixed.

Wasmtime GitHub notifications bot (Sep 29 2020 at 21:49):

abrown merged PR #2240.

Last updated: Nov 22 2024 at 16:03 UTC