Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x64: Add non-SSE4.1 lowerings of pmov{s,z}x* #6279

Merged
merged 2 commits into from
Apr 27, 2023

Conversation

alexcrichton
Copy link
Member

This commit adds lowerings for a suite of sign/zero extension instructions which don't require SSE4.1. Like before these lowerings are based on LLVM's output.

This commit also deletes special casees for i16x8.extmul_{low,high}_* since the output of the special case is the same as the default lowering of all the component instructions used within as well.

This commit adds lowerings for a suite of sign/zero extension
instructions which don't require SSE4.1. Like before these lowerings are
based on LLVM's output.

This commit also deletes special casees for `i16x8.extmul_{low,high}_*`
since the output of the special case is the same as the default lowering
of all the component instructions used within as well.
@alexcrichton alexcrichton requested a review from abrown April 24, 2023 19:27
@alexcrichton alexcrichton requested a review from a team as a code owner April 24, 2023 19:27
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen labels Apr 24, 2023
@elliottt elliottt removed the request for review from a team April 25, 2023 17:04

;; Same as `uwiden_high`, but interleaving high lanes instead.
(rule (lower (has_type $I16X8 (uwiden_high val @ (value_type $I8X16))))
(x64_punpckhbw val (xmm_zero $I8X16)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't remember all the history of PALIGNR + PMOVZX above but this PUNPCKH might be a better lowering even in the SSE 4.1 case (fewer dependencies?). Just a thought, haven't looked too closely at this...

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe llvm-mca agrees with you, and LLVM appears to, even when I use the intrinsics for palignr + pmovzx, to still use the pupckh combo with an xor-to-create-zero.

@abrown
Copy link
Contributor

abrown commented Apr 26, 2023

Also, just a general thought: how can we check that fuzzing is using all of these new <SSE4.1 lowerings?

LLVM prefers the `punpckh*`-based lowerings and at least according to
`llvm-mca` these are slightly better cycle-wise too.
@alexcrichton
Copy link
Member Author

Currently there's no fuzz coverage due to this but at the end of this road I'll be able to disable that meaning we'll get fuzz coverage.

Once that's removed though we should get fuzz coverage. For example after #6206 the pcmpgtq instruction is now conditional. Looking at yesterday's coverage report I found the coverage of the ISLE-generated file and inside of constructor_x64_pcmpgt I see x64_pcmpgtq is called 317 times with the fallback being called 7 times (direct link). Not a huge number of executions but still there!

@alexcrichton alexcrichton enabled auto-merge April 27, 2023 01:43
@alexcrichton alexcrichton added this pull request to the merge queue Apr 27, 2023
Merged via the queue into bytecodealliance:main with commit edae6c0 Apr 27, 2023
@alexcrichton alexcrichton deleted the x64-no-pmovszx branch April 27, 2023 02:54
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Apr 28, 2023
* x64: Add non-SSE4.1 lowerings of `pmov{s,z}x*`

This commit adds lowerings for a suite of sign/zero extension
instructions which don't require SSE4.1. Like before these lowerings are
based on LLVM's output.

This commit also deletes special casees for `i16x8.extmul_{low,high}_*`
since the output of the special case is the same as the default lowering
of all the component instructions used within as well.

* Remove SSE4.1 specialization of `uwiden_high`

LLVM prefers the `punpckh*`-based lowerings and at least according to
`llvm-mca` these are slightly better cycle-wise too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants