llvm dev - Oct 2017 - RFC: AArch64 SVE Assembler/Disassembler patches

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


On 19/10/2017, 14:44, "Alex Bradbury" <asb at lowrisc.org>
wrote:

    On 19 October 2017 at 05:12, Sander De Smalen via llvm-dev
    <llvm-dev at lists.llvm.org> wrote:
    > 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.


On 19/10/2017 16:42, Sander De Smalen via llvm-dev
wrote:
> 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

On 19 October 2017 at 17:58, Florian Hahn via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> I think it would be good to give a description/explanation of the change in
> the review description.
+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