Stream: git-wasmtime

Topic: wasmtime / PR #2813 x64: match multiple ISA requirements ...

Wasmtime GitHub notifications bot (Apr 07 2021 at 23:06):

abrown opened PR #2813 from inst-format-2 to main:

Because there are instructions that are present in more than one ISA feature set, we need to see if any of them match before emitting. This change includes the VPABSQ instruction as an example, which is present in both AVX512F and AVX512VL.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 07 2021 at 23:06):

abrown requested cfallin for a review on PR #2813.

Wasmtime GitHub notifications bot (Apr 07 2021 at 23:06):

abrown requested cfallin and bnjbvr for a review on PR #2813.

Wasmtime GitHub notifications bot (Apr 07 2021 at 23:07):

abrown edited PR #2813 from inst-format-2 to main:

Because there are instructions that are present in more than one ISA feature set, we need to see if any of the ISA requirements match before emitting. This change includes the VPABSQ instruction as an example, which is present in both AVX512F and AVX512VL.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 07 2021 at 23:09):

abrown updated PR #2813 from inst-format-2 to main.

Wasmtime GitHub notifications bot (Apr 07 2021 at 23:29):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Apr 07 2021 at 23:29):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Apr 07 2021 at 23:29):

cfallin created PR Review Comment:

Perhaps we could rename this to something like present_in_isas()? The OR semantics weren't immediately clear to me from the name until I read the code around the callsite above; otherwise I would have expected AND.

Wasmtime GitHub notifications bot (Apr 08 2021 at 07:49):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2021 at 07:49):

bnjbvr created PR Review Comment:

uber nit: do you mind writing it in the final reduced form, if !isa.is_empty() && !isa.iter().any(matches_isa_flags)?

Wasmtime GitHub notifications bot (Apr 08 2021 at 07:49):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2021 at 07:49):

bnjbvr created PR Review Comment:

Or how about present_in_any_isa()? (or available_in_any_isa()?) The "any" in the name makes the OR semantics crystal clear in my opinion.

Also nit: considering a SmallVec<[T; 1]> will cause a heap allocation for the second element, and we only have the cases for 1 and 2 elements, could this return SmallVec<[T; 2]> (or 4; an initial Vec push will reserve capacity for 4 elements)?

Wasmtime GitHub notifications bot (Apr 08 2021 at 15:56):

abrown updated PR #2813 from inst-format-2 to main.

Wasmtime GitHub notifications bot (Apr 08 2021 at 15:57):

abrown updated PR #2813 from inst-format-2 to main.

Wasmtime GitHub notifications bot (Apr 08 2021 at 15:59):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Apr 08 2021 at 15:59):

abrown created PR Review Comment:

Went with available_in_any_isa, added some documentation to that function, and updated to use SmallVec<[T; 2]>.

Wasmtime GitHub notifications bot (Apr 08 2021 at 17:30):

abrown merged PR #2813.

Last updated: Nov 22 2024 at 17:03 UTC