[PATCH] D83734: [WebAssembly] Lower vselect to v128.bitselect

[Thomas Lively via Phabricator via llvm-commits]

tlively marked an inline comment as done.
tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:161
+    // TODO: Implement SELECT_V128
+    for (auto Op : {ISD::SELECT_CC, ISD::SELECT})
       for (auto T : {MVT::v16i8, MVT::v8i16, MVT::v4i32, MVT::v4f32, MVT::v2i64,
----------------
aheejin wrote:
> aheejin wrote:
> > Maybe I don't remember the context very well, but I have some questions:
> > 
> > - Why were we expanding `ISD::VSELECT` until now, given that we had `v128.bitselect`?
> > 
> > - Maybe I don't remember the context, but why do we expand `ISD::SELECT`? [[ https://github.com/WebAssembly/simd/blob/master/proposals/simd/SIMD.md#bitwise-select | This ]] says
> > > Note that the normal WebAssembly select instruction also works with vector types. It selects between two whole vectors controlled by a single scalar value, rather than selecting bits controlled by a control mask vector.
> > Then can't we use wasm's `select` instruction for this?
> > 
> > - What is `SELECT_V128`?
> Oh I now see Q2 is implemented in D83737.
We were expanding ISD::VSELECT until now because I mistakenly thought there was some difference in semantics between ISD::VSELECT and v128.bitselect. The confusion came from the part of the VSELECT documentation that says "The condition follows the BooleanContent format of the target." I probably hadn't realized at the time that this meant the separate BooleanVectorContent, which we set to ZeroOrNegativeOneBooleanContent. We set the non-vector BooleanContent to ZeroOrOneBooleanContent, and if the condition lanes were zero or one, v128.bitselect would not work properly.

I'm not sure why we were expanding ISD::SELECT until now, but it's possible that select on v128 values wasn't implemented in V8 yet when I first implemented this. As you mentioned, ISD::SELECT support is implemented in a follow-on patch, and SELECT_V128 is meant to refer to that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83734/new/

https://reviews.llvm.org/D83734