Page MenuHomePhabricator

[AArch64][SVE] Folds VSELECT if the predicate is all active.
ClosedPublic

Authored by sdesmalen on Jan 25 2022, 7:46 AM.

Details

Summary

This adds the following changes:

  • Fold: vselect(<all active predicate>, x, y) => x
  • Extend isAllActivePredicate to take vscale_range into account, e.g. isAllActivePredicate(vl16) for nxv16i1 and vscale == 1 => true. isAllActivePredicate(vl32) for nxv16i1 and vscale == 2 => true.

Diff Detail

Event Timeline

sdesmalen created this revision.Jan 25 2022, 7:46 AM
sdesmalen requested review of this revision.Jan 25 2022, 7:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2022, 7:46 AM
Matt added a subscriber: Matt.Jan 25 2022, 3:24 PM
david-arm added inline comments.Jan 27 2022, 1:19 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
15128

nit: Maybe this is just personal preference, but I think these types of comparisons are a bit easier to read with (), i.e. PatNumElts == (NumElts * VScale).

15128

It looks like we can also support the case where PatNumElts > (NumElts * VScale) as well, right?

16819

This is purely an observation, but we can also do something similar for all-false predicates by selecting y from (x, y) inputs. I imagine this is much less commonly seen in practice though ...

16828

This is not something introduced by your patch (but if you choose to fix it then great!), but we call N->getOperand(0) twice and have two different variables for it - N0 and SetCC. It's a bit confusing!

sdesmalen updated this revision to Diff 403630.Jan 27 2022, 6:22 AM
sdesmalen marked an inline comment as done.
  • Added case for 'all inactive'.
  • Improved tests by swapping the arguments so that it's clear which value is returned. (for positive tests, it should return the value from z1)
  • Added negative test as well.
sdesmalen marked 2 inline comments as done.Jan 27 2022, 6:24 AM
sdesmalen added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
15128

I wasn't sure if this case would be considered a bug, because if you run for a VL16 on a machine where only VL8 would be legal, then the rest of the code may be wrong as well?

david-arm accepted this revision.Jan 27 2022, 7:37 AM

LGTM! Thanks for making the changes @sdesmalen.

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

nit: Could you delete this before merging as I think it's unused?

This revision is now accepted and ready to land.Jan 27 2022, 7:37 AM
This revision was landed with ongoing or failed builds.Jan 27 2022, 7:59 AM
This revision was automatically updated to reflect the committed changes.
sdesmalen marked an inline comment as done.