Page MenuHomePhabricator

[WebAssembly] Lower vselect to v128.bitselect
ClosedPublic

Authored by tlively on Jul 13 2020, 6:43 PM.

Details

Summary

We were previously expanding vselect and matching on the expansion to
generate bitselects, but in some cases the expansion would be further
combined and a bitselect would not get generated. This patch improves
codegen in those cases by legalizing vselect and lowering it to
v128.bitselect. The old pattern that matches the expansion is still
useful for lowering IR that already uses the expansion rather than a
select operation.

Diff Detail

Event Timeline

tlively created this revision.Jul 13 2020, 6:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2020, 6:43 PM
aheejin accepted this revision.Jul 15 2020, 9:12 PM
aheejin added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
161

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? 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?
This revision is now accepted and ready to land.Jul 15 2020, 9:12 PM
aheejin added inline comments.Jul 15 2020, 11:33 PM
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
161

Oh I now see Q2 is implemented in D83737.

tlively marked an inline comment as done.Jul 16 2020, 11:00 AM
tlively added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
161

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.

This revision was automatically updated to reflect the committed changes.