This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Bitselect intrinsic and instruction
ClosedPublic

Authored by tlively on Oct 2 2018, 3:19 PM.

Diff Detail

Event Timeline

tlively created this revision.Oct 2 2018, 3:19 PM
dschuff added inline comments.Oct 2 2018, 5:46 PM
test/CodeGen/WebAssembly/simd-arith.ll
2

I assume you removed this because you didn't add fast-isel support? You can still keep it as long as you don' have the -fast-isel-abort flag, because if a block cannot be selected with fast-isel it will fall back to SelectionDAG.

tlively added inline comments.Oct 2 2018, 5:48 PM
test/CodeGen/WebAssembly/simd-arith.ll
2

Good to know! I'm not sure that's useful for a test, though, since we want to be explicitly testing fast-isel or not testing it at all.

dschuff added inline comments.Oct 2 2018, 5:58 PM
test/CodeGen/WebAssembly/simd-arith.ll
2

In that case we should add -fast-isel-abort to the llc command lines where we have -fast-isel.

dschuff added inline comments.Oct 2 2018, 6:00 PM
test/CodeGen/WebAssembly/simd-arith.ll
2

And, even if you don't have fast-isel implemented for some of these instructions, it can still be useful to have a test that allows fallback. If the codegen is different between fast and DAG ISel, then the test actually will fail if fast-isel bails unexpectedly. And even if not you can still cover the code (which could catch asserts, etc) even if you can't ensure that it runs

aheejin added inline comments.Oct 3 2018, 9:05 AM
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
971

How about adding newlines between categories, like default / bitselect / anytrue+alltrue / lsda ?

lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
203

Maybe start (vec_t ... from the next line?

466

Does this handle the case of all different commutative permutations of or and and? like,

  • (v1 & c) | (v2 & ~c)
  • (v1 & c) | (~c & v2)
  • (c & v1) | (~c & v2)
  • (c & v1) | (v2 & ~c)
  • (v2 & ~c) | (v1 & c)
  • (~c & v2) | (v1 & c)
  • (~c & v2) | (c & v1)
  • (v2 & ~c) | (c & v1)

It would be also good to add tests for some of other patterns too.

tlively updated this revision to Diff 168149.Oct 3 2018, 11:46 AM
tlively marked 2 inline comments as done.
  • Address comments
lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
971

sgtm

lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
466

yes, thankfully. I rewrote the existing tests so that each one tests a different interesting pattern in addition to a different type.

tlively updated this revision to Diff 168153.Oct 3 2018, 11:51 AM
  • Whitespace changes
aheejin accepted this revision.Oct 3 2018, 2:37 PM
This revision is now accepted and ready to land.Oct 3 2018, 2:37 PM
This revision was automatically updated to reflect the committed changes.