RFC: AArch64 SVE Assembler/Disassembler patches

Hi,

Probably a lot of you are attending interesting talks at LLVM Dev meeting this week, so I hope this message isn’t completely lost in all the excitement.

In the past month we have carved off our changes to LLVM’s assembler/disassembler that implement the AArch64 SVE instruction set [1]. These changes are split these up into individual patches that purely focus on the assembler and disassembler and have no link to the IR (yet). We would like to start sharing these patches with upstream LLVM.

I have made a best effort to split these changes into small, manageable patches (or patch sets) that can be reviewed and applied individually over time, where each patch/patch-set aims to add a new instruction (or variant or addressing mode for an instruction). Each patch-set has corresponding tests to cover the added instruction.

A first set of patches that implement SVE (unpredicated) ADD/SUB instructions can be found in Phabricator:

Please let me know if you have any comments or suggestions to make sharing/reviewing these patches easier. I’m open to any feedback to get patches in the right shape!

Sander

[1] The AArch64 SVE specification and related documents can be found here:

https://developer.arm.com/products/software-development-tools/hpc/sve

Hi Sander, I'm glad to see progress towards getting SVE support
upstream. Compared to codegen, I would hope it's really quite easy to
get these reviewed and merged (i.e. I don't see any reason why it
should be more difficult than the AArch64v8.3 additions). From a quick
skim of the patchset, it seems sensible (AArch64 regulars can review
the fine details - I added a few comments on the last patch). Was
there a specific aspect of the patches + approach taken that you were
looking for feedback on?

Best,

Alex

Thanks Alex!

We thought it would be good to start sharing the more mechanical parts of our SVE work, separate from more involved topics like IR and Codegen. Hopefully the assembler/disassembler patches will give some visibility into the available instructions (other than just pointing to specification/documentation). Most of these assembler/disassembler patches are functionally quite simple, the only thing is that there is a quite a lot of them, so it may take some time before all are reviewed/merged.

Perhaps one of the things I'm hoping to get feedback is if these patches are of the right size and have the right level of description. For instance, I wasn't sure if splitting out e.g. patch 4 (out of 5) is any useful. But I’m open to any feedback people may have.

Other than that, I also wanted to inform about the series of patches that I’ll be putting on Phabricator the coming months with some pointers to the SVE spec (for reference).

Sander

    > Hi,
    >
    >
    >
    > Probably a lot of you are attending interesting talks at LLVM Dev meeting
    > this week, so I hope this message isn't completely lost in all the
    > excitement.
    >
    >
    >
    > In the past month we have carved off our changes to LLVM's
    > assembler/disassembler that implement the AArch64 SVE instruction set [1].
    > These changes are split these up into individual patches that purely focus
    > on the assembler and disassembler and have no link to the IR (yet). We would
    > like to start sharing these patches with upstream LLVM.
    >
    >
    >
    > I have made a best effort to split these changes into small, manageable
    > patches (or patch sets) that can be reviewed and applied individually over
    > time, where each patch/patch-set aims to add a new instruction (or variant
    > or addressing mode for an instruction). Each patch-set has corresponding
    > tests to cover the added instruction.
    >
    >
    >
    > A first set of patches that implement SVE (unpredicated) ADD/SUB
    > instructions can be found in Phabricator:
    >
    > - https://reviews.llvm.org/D39087
    >
    > - https://reviews.llvm.org/D39088
    >
    > - https://reviews.llvm.org/D39089
    >
    > - https://reviews.llvm.org/D39090
    >
    > - https://reviews.llvm.org/D39091
    >
    >
    >
    > Please let me know if you have any comments or suggestions to make
    > sharing/reviewing these patches easier. I'm open to any feedback to get
    > patches in the right shape!

    Hi Sander, I'm glad to see progress towards getting SVE support
    upstream. Compared to codegen, I would hope it's really quite easy to
    get these reviewed and merged (i.e. I don't see any reason why it
    should be more difficult than the AArch64v8.3 additions). From a quick
    skim of the patchset, it seems sensible (AArch64 regulars can review
    the fine details - I added a few comments on the last patch). Was
    there a specific aspect of the patches + approach taken that you were
    looking for feedback on?

    Best,

    Alex

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Hi,

Thanks for sharing the initial patches Sander and thanks Alex for taking a look already!

Also, Graham and myself will host a Hackers Lab table to discuss how to best add support for SVE to LLVM today from 4:40pm - 6:25pm.

Thanks Alex!

We thought it would be good to start sharing the more mechanical parts of our SVE work, separate from more involved topics like IR and Codegen. Hopefully the assembler/disassembler patches will give some visibility into the available instructions (other than just pointing to specification/documentation). Most of these assembler/disassembler patches are functionally quite simple, the only thing is that there is a quite a lot of them, so it may take some time before all are reviewed/merged.

Perhaps one of the things I'm hoping to get feedback is if these patches are of the right size and have the right level of description. For instance, I wasn't sure if splitting out e.g. patch 4 (out of 5) is any useful. But I’m open to any feedback people may have.

I think it would be good to give a description/explanation of the change in the review description.

Other than that, I also wanted to inform about the series of patches that I’ll be putting on Phabricator the coming months with some pointers to the SVE spec (for reference).

Cheers,
Florian

+1, it's really hard to understand the intention just by the title.

A few tips:
- Add links between the patchs in phab, helps review the whole series
in one go. (Edit Related Revisions).
- Describe what the single patch does. If there are things that it
should do but are in another patch, mention that.
- In the first patch, you can be a bit more creative. Describe what
the series do in addition to the first one. Add the following patches
numbers in the comments, too, so people can click directly at the
header.

cheers,
--renato