This is an archive of the discontinued LLVM Phabricator instance.

[LegalizeTypes] Replace vecreduce_xor/or/and with vecreduce_add/umax/umin if not legal
ClosedPublic

Authored by bsmith on Jul 1 2022, 7:53 AM.

Details

Summary

This is done during type legalization since the target representation of
these nodes may not be valid until after type legalization, and after
type legalization the fact that these are dealing with i1 types may be
lost.

Diff Detail

Event Timeline

bsmith created this revision.Jul 1 2022, 7:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2022, 7:53 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
bsmith requested review of this revision.Jul 1 2022, 7:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2022, 7:53 AM
bsmith updated this revision to Diff 441698.Jul 1 2022, 8:19 AM
bsmith retitled this revision from [LegalizeTypes] Replace vecreduce_xor/or with vecreduce_add/umax if not legal to [LegalizeTypes] Replace vecreduce_xor/or/and with vecreduce_add/umax/umin if not legal.
  • Add support for i1 vecreduce_and -> vecreduce_umin.
paulwalker-arm accepted this revision.Jul 5 2022, 5:01 PM

Looks good to me but with a couple of observations.

llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
2273

Perhaps this should be else if? Same goes for the next if block.

llvm/test/CodeGen/AArch64/reduce-and.ll
169–171 ↗(On Diff #441698)

Are the _ins_ tests strictly necessary? It's not immediately obvious how they relate to your code changes.

This revision is now accepted and ready to land.Jul 5 2022, 5:01 PM
bsmith added inline comments.Jul 6 2022, 2:29 AM
llvm/test/CodeGen/AArch64/reduce-and.ll
169–171 ↗(On Diff #441698)

The rest of the tests in these files cause a zero_extend to be generated, hence would be covered by the current way this was being done (in an add dagcombine), these _ins_ tests cause an any_extend to be generated, which is the reason why this functionality was moved to type legalisation, hence it seemed reasonable to test that case also (i.e. I want to avoid someone moving this back to something like an add dagcombine and thinking everything is fine).

paulwalker-arm added inline comments.Jul 6 2022, 2:54 AM
llvm/test/CodeGen/AArch64/reduce-and.ll
169–171 ↗(On Diff #441698)

Sounds reasonable. Thanks for the explanation.

This revision was landed with ongoing or failed builds.Jul 7 2022, 2:48 AM
This revision was automatically updated to reflect the committed changes.