This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] add fold for vselect of signbit
ClosedPublic

Authored by spatel on Nov 4 2021, 12:34 PM.

Details

Summary

(X s< 0) ? Y : 0 --> (X s>> BW-1) & Y

We canonicalize to the icmp+select form in IR, and we already have this fold for scalar select in SDAG, so I think it's an oversight that we don't have the fold for vectors. It seems neutral for AArch64 and saves some instructions on x86.

Whether we should also have the sibling folds for the inverse condition or all-ones true value may depend on target-specific factors such as whether there's an "and-not" instruction.

Diff Detail

Event Timeline

spatel created this revision.Nov 4 2021, 12:34 PM
spatel requested review of this revision.Nov 4 2021, 12:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2021, 12:34 PM
This comment was removed by xbolva00.
RKSimon added inline comments.Nov 5 2021, 3:47 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9566

Would it be useful to have the inverse of this if the target hasAndNot() ?

Thanks! I've added an additional testcase for MVE in rGcb62c3761fe7. (Which is less instructions but may technically be slower now that I think about it - depending on if the vmov is loop invariant and how the microachitecture handles overlap. You can ignore that though - I just wanted to make sure there were some tests for an architecture with first class vector predicates, and any performance gains/losses should be minor and dependent a lot on the surrounding code. They are less instructions in the general case).

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9555

I find it easier to read with one definition per line.

spatel marked an inline comment as done.Nov 5 2021, 4:50 AM
spatel added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9566

Yes - I hinted at this in the patch description and plan to follow-up with that and possibly a couple of other patterns based on the scalar folds.

There are potentially several edge/weird cases though (my recent codegen patches for saturating sub were trying to avoid regressions on this set of folds). So I was planning to try these one at a time.

spatel updated this revision to Diff 385037.Nov 5 2021, 5:13 AM
spatel marked 2 inline comments as done.

Patch updated:

  1. Rebased with mve test diffs (thanks! - I didn't know how to exercise that path).
  2. Reformatted variable defs to one-per-line.
dmgreen accepted this revision.Nov 5 2021, 5:36 AM

Thanks

This revision is now accepted and ready to land.Nov 5 2021, 5:36 AM
RKSimon accepted this revision.Nov 5 2021, 7:06 AM

LGTM - thanks

This revision was landed with ongoing or failed builds.Nov 5 2021, 7:09 AM
This revision was automatically updated to reflect the committed changes.