[PATCH] D35307: [AArch64] Initial SVE register definitions

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

In https://reviews.llvm.org/D35307#807706, @aemerson wrote:

> > This seems a very raw change, without any further description, comments or proper usage, other than a few changes on random places.
>
> I don't agree. The additions are in line with the level of documentation in the rest of the file.


I meant the review description / commit message. It's customary to add links to the documentation that describes the additions and a short text on what's the future plan, etc.

> This is essentially an NFC change until we begin to add support for the actual instructions. To do so, first we need some minimal registers and regclasses defined.

We normally use the term NFC for refactory, not dead code. This only has "no functional changes" because the code is not used at all.

Adding the register classes is an important first step, of course, but they normally come followed by the sequence of commits that will use it, showing their usage, how they'll be tested, etc. Look at the submissions of new back-ends (AArch64, BPF, RISCV, AAP, etc), they're all good examples on how to submit a big change in the back-end, which this is one (SVE).

> The architecture reference manual supplement describes SVE in more detail: https://developer.arm.com/docs/ddi0584/latest/arm-architecture-reference-manual-supplement-the-scalable-vector-extension-sve-for-armv8-a
>  We do not have a full release of the ARMARM to reference yet.

Right, adding that to the description would have been good.

>> Is it not possible to write tests for them? If so, why not?
> 
> Unless the registers are used in an instruction, I'm not aware of any way to test them. Happy to do so if you have a suggestion on this though.

Precisely why you need to show a larger change-set.

>> Is this the first of a series? If yes, there what's the rest? A description of the changes would help, but much better if you pointer to more reviews in the series.
> 
> Not really, the rest of the patches will come eventually but we're waiting on foundations and infrastructure to committed first before we prepare next steps. We're trying to break down the entire SVE support into small pieces.

Carrying dead code for an eventual further patch doesn't scale. Either this patch has a purpose and you show it, or it will just sit here, not being reviewed.

Breaking down SVE into pieces is the right thing to do, but unless you show all the pieces and how they fit together, getting someone to review one piece alone will be impossible.

I suggest you start with at least three major change-sets:

1. Graham's existing IR changes (https://reviews.llvm.org/D32737), which is stuck on more info, even though we had two large RFCs on the list.
2. This change to the back-end, which is stuck because it's dead code.
3. Infrastructural changes in Clang and Target Parser, which proceed because they're unrelated, but have very limited usability.

For both 1 and 2 threads I suggest you guys to show more of the patch set, with a complete block of functionality implemented and tested, from begin to end. That'll help people to understand a larger part of what you're trying to upstream and why it makes sense (or if not, how to make it better, etc).

cheers,
--renato


Repository:
  rL LLVM

https://reviews.llvm.org/D35307