This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Implement abs and neg intrinsics
ClosedPublic

Authored by c-rhodes on Aug 8 2019, 2:31 AM.

Details

Summary

This patch implements two arithmetic intrinsics:

  • int_aarch64_sve_abs
  • int_aarch64_sve_neg

testing the support for scalable vector types in intrinsics added in D65930.

Diff Detail

Repository
rL LLVM

Event Timeline

c-rhodes created this revision.Aug 8 2019, 2:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 2:31 AM
greened added inline comments.Aug 13 2019, 10:17 AM
include/llvm/IR/IntrinsicsAArch64.td
756 ↗(On Diff #214091)

What does "AdvSIMD" reference? Why not "SVE?"

@greened sorry for the delayed response, thanks for taking a look at this.

include/llvm/IR/IntrinsicsAArch64.td
756 ↗(On Diff #214091)

AdvSIMD is NEON, some of the existing AdvSIMD patterns worked fine for our needs (and some of the new ones weren't exactly SVE specific) so we kept the naming scheme. The SVE specific intrinsics we have downstream are prefixed with AdvSIMD_SVE. Happy to change this if it's confusing.

Is it worth adding in ISD::ABS support here ?

c-rhodes added a comment.EditedAug 19 2019, 8:59 AM

Is it worth adding in ISD::ABS support here ?

The scope of this patch is to add codegen support for the intrinsics now defined in the earlier patch (I still need to update this patch to remove the intrinsic def) to give us what we need on the LLVM side to start supporting the Arm C Language Extensions (ACLE) intrinsics, for which we don't need any changes to match generic ISD nodes at the moment. We plan to start upstreaming more generic codegen later.

greened added inline comments.Sep 3 2019, 7:06 AM
include/llvm/IR/IntrinsicsAArch64.td
756 ↗(On Diff #214091)

It is a bit jarring to see the "SVE" comment immediately above and then see this definition with "AdvSIMD." Maybe remove the "SVE" comment since it's not exclusive to SVE?

c-rhodes updated this revision to Diff 218615.Sep 4 2019, 2:15 AM
  • Rebased since parent patch landed.
  • Moved SVE comment after @greened suggestion.
c-rhodes marked an inline comment as done.Sep 4 2019, 2:17 AM
c-rhodes added inline comments.
include/llvm/IR/IntrinsicsAArch64.td
756 ↗(On Diff #214091)

I've moved this comment down to where the SVE intrinsics are defined.

greened accepted this revision.Sep 5 2019, 7:16 AM

LGTM.

This revision is now accepted and ready to land.Sep 5 2019, 7:16 AM
This revision was automatically updated to reflect the committed changes.
c-rhodes marked an inline comment as done.