This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Enable shouldFoldSelectWithIdentityConstant for SVE.
ClosedPublic

Authored by dmgreen on May 5 2023, 8:40 AM.

Details

Summary

Instcombine will canonicalize select(c, binop(a, b), a) to binop(select(c, b, identityvalue), a). The original select form makes a more natural form for vector predicated operations for vector architectures like SVE where predication is well supported. This patch enables shouldFoldSelectWithIdentityConstant for SVE so that more predicated instructions can be generated, helping simplify the handling with identity constants.

Predicated FMA patterns have also been adjusted here as they need to look at FMF's. Other operations like add/sub, mul, and/or/xor and mla/mls I have other patches for, most of which are fairly simple.

There is one test (scalable_int_min_max) that increases in size. There are multiple selects that could be combined into a single select but does not currently fold.

Diff Detail

Event Timeline

dmgreen created this revision.May 5 2023, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2023, 8:40 AM
dmgreen requested review of this revision.May 5 2023, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2023, 8:40 AM

I don't get the argument here. To me the instcombine canonical form is better because it results in reduce operand use counts. I also don't understand how this canonical IR form is less natural for SVE given ISel patterns already exist (albeit not all ops have been covered yet). This just looks like an artificial and unnecessary switch and so perhaps you can share a better rational?

Matt added a subscriber: Matt.May 5 2023, 2:35 PM

The argument may not apply as well to SVE as much as it does to over architectures. The general pattern for a predicated vector operation is select(cond, binop(a, b), passthru), where a==passthru is a special case and gets transformed by instcombine to binop(select(cond, a, identity), b). If the backend already has patterns for the select(binop form then the special case with identity isn't helpful, and it is better to canonicalize it back to select(binop. With SVE, my understanding is that this should often be handled via movpfx if it is fast enough.

I'm not sure I buy the one-use argument. We already know that we have a predicated operation so in general want to produce a single instruction, and that the instructions were not previously optimized by instcombine. Most major architectures already use shouldFoldSelectWithIdentityConstant and it seems unlikely that there would be many folds that we want that were not already handled. Using shouldFoldSelectWithIdentityConstant will be simpler and less error prone, especially around floating point patterns with multiple possible identity values around zero.

paulwalker-arm accepted this revision.May 15 2023, 3:40 AM

Fair enough. It still seems a bit arbitrary but I do agree that it'll make it easier to isel without having to explicitly handle the various floating point identity values. Please don't forget about the bug fix.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
15487

This needs to be hasSVEorSME because we use the same patterns for SME. That said, I'm wondering if it can be emitted altogether given knowing VT is a legal scalable vector should be sufficient.

This revision is now accepted and ready to land.May 15 2023, 3:40 AM
dmgreen updated this revision to Diff 524267.May 22 2023, 6:14 AM

Rebase to use VSelectUnpredOrPassthruPatFrags.

This revision was landed with ongoing or failed builds.Jun 15 2023, 1:17 AM
This revision was automatically updated to reflect the committed changes.