Page MenuHomePhabricator

[AArch64] Initial SVE register definitions
Needs ReviewPublic

Authored by huntergr on Jul 12 2017, 8:09 AM.

Details

Summary

Initial SVE register definitions.

Also extends tablegen to fix type deduction with scalable vectors in patterns, needed in order to build the existing NEON tblgen code with these additional regclasses defined.

Diff Detail

Repository
rL LLVM

Event Timeline

aemerson created this revision.Jul 12 2017, 8:09 AM
fhahn added a subscriber: fhahn.Jul 12 2017, 8:39 AM
fhahn added inline comments.Jul 12 2017, 8:44 AM
lib/Target/AArch64/AArch64RegisterInfo.td
766

Is the comment here intentional?

aemerson updated this revision to Diff 106231.Jul 12 2017, 8:51 AM

Removing comment.

aemerson marked an inline comment as done.Jul 12 2017, 8:52 AM
rengolin edited edge metadata.Jul 12 2017, 9:09 AM

Hi Amara,

This seems a very raw change, without any further description, comments or proper usage, other than a few changes on random places.

You have to describe what the changes are meant to do, where the documents are (as a refresher), which chapters of the documents those registers are described, and hopefully some use of them somewhere.

Is it not possible to write tests for them? If so, why not?

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.

cheers,
--renato

lib/Target/AArch64/AArch64RegisterInfo.td
33

Avoid unnecessary line moves.

134

What's this? The "register" that stores the current length of the vectors?

sdesmalen added inline comments.
lib/Target/AArch64/AArch64RegisterInfo.td
134

The VG Dwarf register indeed contains the length of an SVE vector, defined as the number of 64bit 'granules' in a vector (e.g. 2 for a 16byte vector). It is described in more detail here:
https://developer.arm.com/docs/100985/0000

Hi Amara,

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. 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.

You have to describe what the changes are meant to do, where the documents are (as a refresher), which chapters of the documents those registers are described, and hopefully some use of them somewhere.

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.

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.

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.

willlovett added inline comments.
lib/Target/AArch64/AArch64RegisterInfo.td
766

Yes. Some encodings (eg. single precision FCMLA) restrict the addressable register set (in this case, the second input register) to Z0-15. Similarly the 3-bit version below for half-precision restricts it to Z0-7

Gah. Misunderstanding on my part of what Florian was talking about. Please disregard my previous comment

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 (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

t.p.northover added inline comments.Jul 14 2017, 8:13 AM
lib/Target/AArch64/AArch64RegisterInfo.td
134

Does LLVM need to represent this (maybe for lldb??)? I could see it being plumbed through Uses/Defs like NZCV, but that seems a bit pointless since from what I've heard it's going to be essentially per-process (or near enough).

utils/TableGen/CodeGenDAGPatterns.cpp
546

This is out of date if you're adopting Chris's <scalable 4 x i32> suggestion (which I also quite like).

I'm resigning from SVE upstreaming related activities, so Graham will be taking over this patch and others from here.

huntergr commandeered this revision.Aug 2 2017, 6:34 AM
huntergr added a reviewer: aemerson.
aemerson resigned from this revision.Aug 4 2017, 9:44 AM