This is an archive of the discontinued LLVM Phabricator instance.

[SVE][VLS] Don't combine logical AND.
ClosedPublic

Authored by fpetrogalli on Aug 6 2020, 3:24 PM.

Details

Summary

This patch makes sure that fixed length SVE code generation does not fail trying to generate the NEON bic instruction when operating with vector wider that 128 bits. For vector bigger than 128-bit, the SVE and instruction is used.

The behavior on NEON vectors is preserved and guarded with the two 128-bit and 64-bit tests.

Diff Detail

Event Timeline

fpetrogalli created this revision.Aug 6 2020, 3:24 PM
fpetrogalli requested review of this revision.Aug 6 2020, 3:24 PM

I do not understand the new tests. At first glance they look identical to those in sve-fixed-length-int-log.ll and when I test them against HEAD they pass as is (i.e. without the fix to performANDCombine). I suspect you need a more complex test to exercise the buggy code path.

Whilst discussing the testing I'd prefer to keep the SVE fixed length tests under the sve-fixed-length-* namespace and given the change I doubt you need test files for multiple variants of SVE. If if works for SVE256 then why would it break for larger implementations. You could follow a similar style as sve-fixed-length-subvector.ll or sve-fixed-length-int-log.ll (note the GE LE difference).

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

Does the combine also apply to 64-bit vectors? I know the code it correct but the comment might need updating. If it helps I normally use "NEON sized vectors" or "NEON vectors" to cover all the cases.

I have updated the tests to make sure they are related to the code that has been modified.

fpetrogalli edited the summary of this revision. (Show Details)Aug 7 2020, 9:49 AM
fpetrogalli marked an inline comment as done.Aug 7 2020, 9:49 AM
paulwalker-arm accepted this revision.Aug 10 2020, 7:37 AM

A couple of recommendations to consider before landing the patch.

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

Should people know what VLS means? Personally I think you can drop the reference since "It does not work for SVE when dealing with vectors larger than 128bits." should be clear enough.

11154

At some point we might have a genuine larger than NEON combine here at which point we should protect the NEON specific code with .is64BitVector || is128BitVector, but I guess right now this'll do.

llvm/test/CodeGen/AArch64/vls-sve-512-and.ll
1 ↗(On Diff #283934)

I'm still going to recommend again that you create tests using the sve-fixed-length- prefix so all relevant testing is kept together.

This revision is now accepted and ready to land.Aug 10 2020, 7:37 AM
fpetrogalli marked 2 inline comments as done.

Thank you for the review @paulwalker-arm.

fpetrogalli marked an inline comment as done.Aug 12 2020, 9:08 AM
This revision was automatically updated to reflect the committed changes.