Stream: git-wasmtime

Topic: wasmtime / PR #2095 machinst x64: enable simd_const.wast ...

Wasmtime GitHub notifications bot (Aug 04 2020 at 21:39):

abrown opened PR #2095 from simd-const to main:

This change enables the simd_const.wast spec test and adds encodings for integer vectors and new Inst::[move|load|store] utility functions.

Before merging:

• [ ] this should be rebased on @jlb6740's integer arithmetic changes since simd_const.wast contains a single use each of i64x2.add and i32x4.add
• [ ] I would like feedback on what others think about the Inst::[move|load|store] utility functions: are they the right level of abstraction? Is the de-duplication of code worth the overhead? Should we remove Inst::xmm_mov (and/or others)?

Wasmtime GitHub notifications bot (Aug 04 2020 at 21:39):

abrown requested cfallin for a review on PR #2095.

Wasmtime GitHub notifications bot (Aug 04 2020 at 21:40):

abrown requested cfallin and julian-seward1 for a review on PR #2095.

Wasmtime GitHub notifications bot (Aug 04 2020 at 21:40):

abrown requested cfallin, julian-seward1 and jlb6740 for a review on PR #2095.

Wasmtime GitHub notifications bot (Aug 04 2020 at 22:08):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Aug 04 2020 at 22:08):

abrown created PR Review Comment:

Looking around more, I guess this should just be merged in with gen_move?

Wasmtime GitHub notifications bot (Aug 05 2020 at 00:50):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 05 2020 at 00:50):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 05 2020 at 00:50):

cfallin created PR Review Comment:

I'd prefer not to have function names overlapping with reserved words -- though it's technically possible with this "raw identifier" syntax, it's quite obscure (IMHO at least -- I had to look it up) and potentially causes confusion for no good reason. Perhaps just gen_move?

Wasmtime GitHub notifications bot (Aug 05 2020 at 00:50):

cfallin created PR Review Comment:

I like having helpers on Inst here. However, I think the extension should be an optional behavior; it's easy to imagine reaching for Inst::load() in the future for some general use-case and getting an extension op when it wasn't really needed (e.g. most narrow arithmetic doesn't care about high bits).

Could we define an enum for extension mode, something like the aarch64 backend's NarrowValueMode, to use here?

Wasmtime GitHub notifications bot (Aug 05 2020 at 00:50):

cfallin created PR Review Comment:

A doc comment would be helpful here -- from the name it's not immediately clear what this does (movable to/from a scalar register? movable within a scalar register? ...).

Wasmtime GitHub notifications bot (Aug 05 2020 at 17:29):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Aug 05 2020 at 17:29):

cfallin created PR Review Comment:

Ah, yes, that would actually be the best solution, I think.

Wasmtime GitHub notifications bot (Aug 05 2020 at 17:46):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Aug 05 2020 at 17:46):

abrown created PR Review Comment:

Yeah, I will move this into gen_move.

Wasmtime GitHub notifications bot (Aug 07 2020 at 17:17):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Aug 07 2020 at 17:17):

abrown created PR Review Comment:

Got rid of this and used a comparison on RegClass instead.

Wasmtime GitHub notifications bot (Aug 07 2020 at 17:45):

abrown updated PR #2095 from simd-const to main:

This change enables the simd_const.wast spec test and adds encodings for integer vectors and new Inst::[move|load|store] utility functions.

Before merging:

• [ ] this should be rebased on @jlb6740's integer arithmetic changes since simd_const.wast contains a single use each of i64x2.add and i32x4.add
• [ ] I would like feedback on what others think about the Inst::[move|load|store] utility functions: are they the right level of abstraction? Is the de-duplication of code worth the overhead? Should we remove Inst::xmm_mov (and/or others)?

Wasmtime GitHub notifications bot (Aug 07 2020 at 21:35):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Aug 07 2020 at 21:35):

abrown created PR Review Comment:

I added an ExtKind enum; with that in place we should now be able to remove xmm_mov and simplify the lowering considerably in a future PR.

Wasmtime GitHub notifications bot (Aug 07 2020 at 21:35):

abrown requested bnjbvr, julian-seward1 and jlb6740 for a review on PR #2095.

Wasmtime GitHub notifications bot (Aug 07 2020 at 21:35):

abrown edited PR #2095 from simd-const to main:

This change enables the simd_const.wast spec test and adds encodings for integer vectors and new Inst::[move|load|store] utility functions.

Before merging:

• [ ] this should be rebased on @jlb6740's integer arithmetic changes since simd_const.wast contains a single use each of i64x2.add and i32x4.add
• [x] I would like feedback on what others think about the Inst::[move|load|store] utility functions: are they the right level of abstraction? Is the de-duplication of code worth the overhead? Should we remove Inst::xmm_mov (and/or others)?

Wasmtime GitHub notifications bot (Aug 07 2020 at 21:35):

abrown edited PR #2095 from simd-const to main:

This change enables the simd_const.wast spec test and adds encodings for integer vectors and new Inst::[move|load|store] utility functions.

Before merging:

• [x] this should be rebased on @jlb6740's integer arithmetic changes since simd_const.wast contains a single use each of i64x2.add and i32x4.add
• [x] I would like feedback on what others think about the Inst::[move|load|store] utility functions: are they the right level of abstraction? Is the de-duplication of code worth the overhead? Should we remove Inst::xmm_mov (and/or others)?

Wasmtime GitHub notifications bot (Aug 10 2020 at 15:32):

abrown has marked PR #2095 as ready for review.

Wasmtime GitHub notifications bot (Aug 20 2020 at 14:05):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Aug 20 2020 at 19:03):

abrown updated PR #2095 from simd-const to main:

This change enables the simd_const.wast spec test and adds encodings for integer vectors and new Inst::[move|load|store] utility functions.

Before merging:

• [x] this should be rebased on @jlb6740's integer arithmetic changes since simd_const.wast contains a single use each of i64x2.add and i32x4.add
• [x] I would like feedback on what others think about the Inst::[move|load|store] utility functions: are they the right level of abstraction? Is the de-duplication of code worth the overhead? Should we remove Inst::xmm_mov (and/or others)?

Wasmtime GitHub notifications bot (Aug 20 2020 at 19:37):

abrown merged PR #2095.

Last updated: Nov 22 2024 at 16:03 UTC