Skip to content
This repository has been archived by the owner on Dec 22, 2021. It is now read-only.

Replace i8x16/i16x8/i32x4.any_true with v128.any_true #423

Merged
merged 1 commit into from
Jan 12, 2021

Conversation

Maratyszcza
Copy link
Contributor

This patch implements the proposal #416 to make any_true instruction untyped as semantically its operation is independent of lane types.

@Maratyszcza
Copy link
Contributor Author

This proposal was voted on and accepted in the 1/8/2021 meeting.

Copy link
Contributor

@abrown abrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@dtig dtig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this! Lgtm with nit.

@@ -82,7 +82,6 @@
| -------------------- | ------ | ------------------------ | ------ | ------------------------ | ------ | ------------- | ------ |
| i8x16.abs | 0x60 | i16x8.abs | 0x80 | i32x4.abs | 0xa0 | ---- | 0xc0 |
| i8x16.neg | 0x61 | i16x8.neg | 0x81 | i32x4.neg | 0xa1 | i64x2.neg | 0xc1 |
| i8x16.any_true | 0x62 | i16x8.any_true | 0x82 | i32x4.any_true | 0xa2 | ---- | 0xc2 |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with the rest of this document, perhaps leave in "----" for the operations being removed instead of deleting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dtig dtig merged commit c0a5dde into WebAssembly:master Jan 12, 2021
@ngzhian
Copy link
Member

ngzhian commented Jan 12, 2021

This wasn't captured in the meetings notes: i recall us saying we can use i8x16.any_true's opcode for this. Don't we want that? Or do we prefer to group it together with the other v128 logical ops?

@lars-t-hansen
Copy link
Contributor

My memory is the same as @ngzhian's.

Comment on lines +85 to +86
| ------------- | 0x62 | ------------- | 0x82 | ------------- | 0xa2 | ------------- | 0xc2 |
| i8x16.all_true | 0x63 | i16x8.all_true | 0x83 | i32x4.all_true | 0xa3 | ------------- | 0xc3 |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Maratyszcza Hi, I understand the removal of the type specific any_true but should the v128.any_true be associated with an opcode somewhere in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The opcodes is TBD. IIUC, the plan is to assign opcodes for newly added instructions once the instruction set is finalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#421 tracks TBD opcodes.

luyahan pushed a commit to luyahan/v8 that referenced this pull request Jan 26, 2021
In WebAssembly/simd#423, all any_true
instructions were removed, and replaced with a single v128.any_true.

This patch removes all but v8x16.any_true, and renames it to
v128.any_true.

Bug: v8:11331
Change-Id: Ie394ec841a1a1c4030c4f589eac2cee8a6a2a1f9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2639033
Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Deepti Gandluri <gdeepti@chromium.org>
Commit-Queue: Zhi An Ng <zhin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72304}
pull bot pushed a commit to Alan-love/v8 that referenced this pull request Jan 26, 2021
This reverts commit 9c09c22.

Reason for revert: gc stress failures https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux%20-%20arm64%20-%20sim%20-%20gc%20stress/20563/overview

Original change's description:
> [wasm-simd] Merge all any_true to v128.any_true
>
> In WebAssembly/simd#423, all any_true
> instructions were removed, and replaced with a single v128.any_true.
>
> This patch removes all but v8x16.any_true, and renames it to
> v128.any_true.
>
> Bug: v8:11331
> Change-Id: Ie394ec841a1a1c4030c4f589eac2cee8a6a2a1f9
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2639033
> Reviewed-by: Georg Neis <neis@chromium.org>
> Reviewed-by: Deepti Gandluri <gdeepti@chromium.org>
> Commit-Queue: Zhi An Ng <zhin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#72304}

TBR=neis@chromium.org,gdeepti@chromium.org,neis@google.com,zhin@chromium.org

Change-Id: I52dbf8de679059dd7b17908c1fe3ada0eb54ff84
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:11331
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2649240
Reviewed-by: Zhi An Ng <zhin@chromium.org>
Commit-Queue: Zhi An Ng <zhin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72305}
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants