This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Use `shufflevector` for shuffle
AcceptedPublic

Authored by penzn on Dec 29 2022, 11:12 PM.

Details

Reviewers
tlively
Summary

Back out D66983, "[WebAssembly] Add wasm-specific vector shuffle builtin and
intrinsic".

Fix shuffle intrinsic tests. Since they are compiled with -O2 masks need to be
updated as well, otherwise optimizer would replace second operand with
poison.

Convert shuffle intrinsic codegen tests to use shufflevector and move them to
a separate file. This also requires some mask updates to avoid invalid
shufflevector use and to make sure both operands get picked up.

This reverts commit 8e3e56f2a36701480eeb65e426701d5a9025cc59.

Diff Detail

Event Timeline

penzn created this revision.Dec 29 2022, 11:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 29 2022, 11:12 PM
penzn requested review of this revision.Dec 29 2022, 11:12 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 29 2022, 11:12 PM
penzn edited the summary of this revision. (Show Details)Dec 29 2022, 11:13 PM
penzn added a comment.EditedDec 29 2022, 11:17 PM

I still need to run a few more external tests, like the ones mentioned in

https://github.com/emscripten-core/emscripten/issues/9340
https://github.com/WebAssembly/simd/issues/196

penzn edited the summary of this revision. (Show Details)Dec 30 2022, 10:20 PM
tlively accepted this revision.Jan 3 2023, 12:57 PM

LGTM % comment. Thanks for taking this!

llvm/test/CodeGen/WebAssembly/simd-intrinsics.ll
152–153

Since this no longer tests codegen for an intrinsic function, could you move it to a separate test file? It could be named something simple and short like simd-shuffle.ll.

This revision is now accepted and ready to land.Jan 3 2023, 12:57 PM
penzn updated this revision to Diff 486636.Jan 5 2023, 11:08 AM
penzn edited the summary of this revision. (Show Details)
penzn marked an inline comment as done.

Moved the test out.

llvm/test/CodeGen/WebAssembly/simd-shuffle.ll
1–2

Do we need both isel options? I felt bad about removing a test, but we don't check anything specific to the first run line.

tlively accepted this revision.Jan 5 2023, 11:14 AM
tlively added inline comments.
llvm/test/CodeGen/WebAssembly/simd-shuffle.ll
1–2

I think it's good to check with and without fast isel just as a sanity check that fast isel falls back to dag isel correctly, but that's also covered in other tests, so I don't feel strongly about keeping it in this file specifically. Either way is fine with me.