This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Lower fixed length VECREDUCE_AND operation
ClosedPublic

Authored by cameron.mcinally on Oct 1 2020, 3:33 PM.

Details

Summary

Some things to consider:

  1. NEON currently lowers the 64-bit and 128-bit vector AND reductions to a tree based algorithm, IINM. Do we want to go with the NEON lowerings or SVE instructions for those? The i8 and i16 cases should probably use SVE for code clarity. But the v2 vectors may be short enough that NEON is a win. They'll expand to something like:
ext v1.16b, v0.16b, v0.16b, #8
and v0.8b, v0.8b, v1.8b

Are we okay with tuning these later? Or should I do a study now?

  1. If we choose SVE instructions for #1, the OverrideNEON flag is getting bulky again. We might want to consider refactoring that, since we'll need to add more cases for OR and XOR.
  1. I named the new test file sve-fixed-length-log-reduce.ll. The log follows the existing AND tests in /AArch64, but is a little misleading since they're bitwise operations, not logical. Any suggestions on alternative names?

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
cameron.mcinally requested review of this revision.Oct 1 2020, 3:33 PM

Re: OverrideNEON: I wouldn't get too hung up on this. The original intention was me trying to reduce the chances of going down broken code paths and also not surprise people with a complete change of code generation output (a.k.a Operation "get something that's usable quickly"). Once wide vector support is driven by function attributes and we start adding proper v#i1 support, we'll be forced to breakaway from NEON and OverrideNEON will become a distant memory.

we start adding proper v#i1 support

Glad to hear this. Knowing a vector is a mask will go a long way during lowering...

paulwalker-arm accepted this revision.Oct 5 2020, 4:56 AM
paulwalker-arm added inline comments.
llvm/test/CodeGen/AArch64/sve-fixed-length-log-reduce.ll
68

Can you add VBITS_EQ_256 check lines to the VBITS_GE_512 related tests to ensure sensible type legalisation. See sve-fixed-length-int-minmax.ll for example. It looks like I missed this for the other reduction tests.

This revision is now accepted and ready to land.Oct 5 2020, 4:56 AM
This revision was landed with ongoing or failed builds.Oct 5 2020, 9:34 AM
This revision was automatically updated to reflect the committed changes.

Committed. It looks like the legalisations seem reasonable. Something like:

; VBITS_EQ_256-DAG: and [[AND:z[0-9]+]].d, [[LO]].d, [[HI]].d
; VBITS_EQ_256-DAG: andv h[[REDUCE:[0-9]+]], [[PG]], [[AND]].h

How are the legalisation tests usually handled? Are they done once for a class of instructions? Or should I go back to add CHECKs for the other reductions too? @kmclaughlin

Committed. It looks like the legalisations seem reasonable. Something like:

; VBITS_EQ_256-DAG: and [[AND:z[0-9]+]].d, [[LO]].d, [[HI]].d
; VBITS_EQ_256-DAG: andv h[[REDUCE:[0-9]+]], [[PG]], [[AND]].h

How are the legalisation tests usually handled? Are they done once for a class of instructions? Or should I go back to add CHECKs for the other reductions too? @kmclaughlin

In general I've tried to always add VBITS_EQ_256 check lines to any test that also has VBITS_GE_512 check lines.

cameron.mcinally marked an inline comment as done.Oct 5 2020, 2:06 PM

Legalisation tests added for sve-fixed-length-int-reduce.ll in:

6bec45e2558566e10be71280a3e2c1b144f1b236

Legalisation tests added for sve-fixed-length-fp-reduce.ll in:

365ef499d60

Can you update the tests to use the new non-experimental intrinsic name?

Can you update the tests to use the new non-experimental intrinsic name?

Sure. I'll land that today....