This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Canonicalise adds/subs of i1 vectors using XOR
ClosedPublic

Authored by david-arm on Feb 23 2021, 5:45 AM.

Details

Summary

When calling SelectionDAG::getNode() to create an ADD or SUB
of two vectors with i1 element types we can canonicalise this
to use XOR instead, where 1+1 is treated as wrapping around
to 0 and 0-1 wraps to 1.

I've added the following tests for SVE targets:

CodeGen/AArch64/sve-pred-arith.ll

and modified some X86 tests to reflect the much simpler codegen
required.

Diff Detail

Event Timeline

david-arm created this revision.Feb 23 2021, 5:45 AM
david-arm requested review of this revision.Feb 23 2021, 5:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2021, 5:45 AM
craig.topper added inline comments.Feb 23 2021, 9:45 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5312

Isn't this also possible for sub?

david-arm updated this revision to Diff 326014.Feb 24 2021, 1:27 AM
david-arm retitled this revision from [CodeGen] Canonicalise adds of i1 vectors using XOR to [CodeGen] Canonicalise adds/subs of i1 vectors using XOR.
david-arm edited the summary of this revision. (Show Details)
  • Also canonicalised SUB of two predicate vectors to using XOR.
david-arm marked an inline comment as done.Feb 24 2021, 1:28 AM
david-arm added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5312

Yes it is - this is what the X86 target already seems to do. Thanks for the suggestion!

paulwalker-arm accepted this revision.Feb 24 2021, 2:53 AM

Not sure of the value of the "ILLEGAL" tests given they're actually testing we can type legalise ISD::XOR, which I'm presuming is already well tested. That said it's nothing I cannot live with.

This revision is now accepted and ready to land.Feb 24 2021, 2:53 AM
nikic added a subscriber: nikic.

Why is this in getNode() rather than DAGCombine? https://github.com/llvm/llvm-project/commit/6f5a805bbbed5d0cdaaf67846dffa7f044afb407 for example does something very similar in DAGCombine. What's the guideline for correct placement here?

paulwalker-arm added a comment.EditedFeb 24 2021, 3:50 AM

Why is this in getNode() rather than DAGCombine? https://github.com/llvm/llvm-project/commit/6f5a805bbbed5d0cdaaf67846dffa7f044afb407 for example does something very similar in DAGCombine. What's the guideline for correct placement here?

We've followed similar logic as used for i1 based min/max and reductions. My thinking is that the earlier you can canonicalise expressions the better, and you cannot get earlier than never creating them in the first place. Ultimately the goal here is that for some operations there is never a need to consider i1 based vectors. This is partially true today because i1 vectors are rarely a legal type. However i1 vectors are legal for SVE and thus we're hitting corner cases that have not been hit before (for aarch64 at least).

Why is this in getNode() rather than DAGCombine? https://github.com/llvm/llvm-project/commit/6f5a805bbbed5d0cdaaf67846dffa7f044afb407 for example does something very similar in DAGCombine. What's the guideline for correct placement here?

Something that purely depends on the VT should be handled in getNode() - I can't recall the reasons I didn't put it there at the the time

nikic added a comment.Feb 24 2021, 8:22 AM

Thanks for the clarification!

This revision was automatically updated to reflect the committed changes.
david-arm marked an inline comment as done.