This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][NEON] Match (or (and -a b) (and (a+1) b)) => bit select
ClosedPublic

Authored by bsmith on Apr 12 2021, 7:00 AM.

Details

Summary

With this patch vbslq_f32(vnegq_s32(a), b, c) lowers to a BIT instruction.

Co-authored-by: Paul Walker <paul.walker@arm.com>

Diff Detail

Event Timeline

bsmith created this revision.Apr 12 2021, 7:00 AM
bsmith requested review of this revision.Apr 12 2021, 7:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2021, 7:00 AM
dmgreen added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12577

What do the N= and O= signify?

llvm/test/CodeGen/AArch64/neon-bitselect.ll
11

You missed one :-)

bsmith updated this revision to Diff 337092.Apr 13 2021, 3:43 AM
  • Add missing test
  • Tidy up comment
bsmith marked 2 inline comments as done.Apr 13 2021, 3:43 AM
joechrisellis added inline comments.Apr 13 2021, 5:49 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12581–12597

Small suggestion -- it might be possible to get rid of some duplicate code here by doing something like:

if (O0.getOpcode() == ISD::SUB)
    std::swap(O0, O1);

if (O1.getOpcode() != ISD::ADD)
    continue;

// if we get here, O0 is guaranteed to be the sub, and O1 the add
...

If this works, I _think_ it might make things clearer. But I don't know for sure. If it starts to look uglier then I have no problem with it as-is. :)

llvm/test/CodeGen/AArch64/neon-bitselect.ll
3

Can we add this as an attribute instead? 😄

attributes #0 = { "target-features"="+neon" }
bsmith added inline comments.Apr 13 2021, 9:18 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12581–12597

To be honest I think something like this is just going to end up being more confusing, for the sake of saving a couple of duplicated lines, I really don't think it's worth it. Also note that your example doesn't work since the second check isn't correct in the case the operands have been swapped.

joechrisellis added inline comments.Apr 13 2021, 9:45 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12581–12597

You're right! It should have been something like:

if (O0.getOpcode() != ISD::SUB)
    std::swap(O0, O1);

if (O0.getOpcode() != ISD::SUB || O1.getOpcode() != ISD::ADD)
    continue;

// if we get here, O0 is guaranteed to be the sub, and O1 the add
...

... either way, you are right; N0 and N1 make this a bit more awkward than I had anticipated. ACK. 🙂

bsmith updated this revision to Diff 337399.Apr 14 2021, 4:11 AM
  • Use attribute in tests in favour of -mattr on run line
dmgreen accepted this revision.Apr 14 2021, 5:20 AM

I would have gone the other way, making the mtriple and mattr both command line args. LGTM either way though.

This revision is now accepted and ready to land.Apr 14 2021, 5:20 AM
This revision was landed with ongoing or failed builds.Apr 15 2021, 5:53 AM
This revision was automatically updated to reflect the committed changes.