Page MenuHomePhabricator

[AArch64] Implement MC support for Scalable Vector Extension 2 (SVE2)
AbandonedPublic

Authored by c-rhodes on Apr 29 2019, 7:26 AM.

Details

Summary

This patch adds MC support for the entire Arm SVE2 instruction set, along with
flag changes enabling the feature.

All new instructions are defined in the existing AArch64SVEInstrInfo.td and
SVEInstrFormats.td TableGen files, with the latter containing the more generic
multiclasses.

The majority of instructions are enabled with the sve2 feature flag, although
there are 4 additional architecture features enabling other instructions, these
are: sve2-aes, sve2-sm4, sve2-sha3, and bitperm.

For more information please refer to the ISA spec available at:
https://developer.arm.com/docs/ddi0602/latest

Diff Detail

Event Timeline

c-rhodes created this revision.Apr 29 2019, 7:26 AM

This is a really big patch! :-) I think reviewing and committing this would be lot easier if you split this patch up (split it up per instruction or instruction group).

@SjoerdMeijer Yes the patch is indeed quite substantial in size :) In contrast to the original SVE MC patches though, these changes are only new TableGen instruction definitions and encoding classes, corresponding (dis)assembler tests and a few flags. For example, there aren't any code changes to e.g. implement/parse new operand types.

We weren't sure if anyone would actually be interested in reviewing the encodings in detail and figured it may be more of a reference we could point to for those in the LLVM community that follow the work on AArch64's Scalable Vector Extensions and are interested in the instructions added in SVE2. Perhaps that would have been worth a comment in the summary/description :)

The encodings have been quite extensively tested and independently verified.

If you still think there is an argument to split up the patch for reviewing purposes, naturally we'd be happy to do so!

Oh, I am definitely not interested in checking encodings. Machines are indeed much better in that.

But glancing over the patches would still be useful I think, i.e. the perhaps non-trivial tablegen parts. I think the patches that at least need to be split up are the parts that deal with the TargetParser, AsmParser, etc. With SVE, we still had quite some comments, even though they were mostly minor, but still. But at the moment I can't even look at the code, the patch is too big. And then yes, you can probably start committing all the tablegen discriptions in one go, like you did with SVE.

Perhaps good to get some opinions from @olista01 and @t.p.northover on this too.

javed.absar added inline comments.Apr 30 2019, 2:45 AM
lib/Target/AArch64/AArch64SchedExynosM1.td
27

Instead of repeating this list everywhere you can at top level :

class AArch64UnsupportedF { list<Predicate> F; }
def SVEUnsupported F : AArch64UnsupportedF {

let F = [HasSVE, HasSVE2, ... ];

}

and then in each AArch64SchedX.td ::
list<Predicate> UnsupportedFeatures = SVEUnsupported.F;

TableGen curently does not have globals otherwise it would be simpler, but this idiom works

lib/Target/AArch64/AArch64SchedExynosM1.td
27

That's neat!

I presume top-level is AArch64.td, I'm happy to make the change there

But glancing over the patches would still be useful I think, i.e. the perhaps non-trivial tablegen parts. I think the patches that at least need to be split up are the parts that deal with the TargetParser, AsmParser, etc.

The changes to the TargetParser, AsmParser and schedulers are adding new feature support, I think that constitutes one patch and I can split that out.

With SVE, we still had quite some comments, even though they were mostly minor, but still. But at the moment I can't even look at the code, the patch is too big. And then yes, you can probably start committing all the tablegen discriptions in one go, like you did with SVE.

If I split the above into a separate patch the remaining changes will be TableGen descriptions adding new instructions, along with tests. I understand this will still be a huge patch to review so I could break it down by encoding group (~26) as you suggest if that will make things easier.

I don't have strong opinions on this:

If I split the above into a separate patch the remaining changes will be TableGen descriptions adding new instructions, along with tests. I understand this will still be a huge patch to review so I could break it down by encoding group (~26) as you suggest if that will make things easier.

This could still be one big bang commit, it's mostly tests (and encodings) and I assume too that people will believe this. But yes, this would be helpful I think:

The changes to the TargetParser, AsmParser and schedulers are adding new feature support, I think that constitutes one patch and I can split that out.

Yes, from a closer look, I think too that this is a separate patch. Perhaps the only one, indeed, but it was a bit difficult to see :-)

The changes to the TargetParser, AsmParser and schedulers are adding new feature support, I think that constitutes one patch and I can split that out.

Yes, from a closer look, I think too that this is a separate patch. Perhaps the only one, indeed, but it was a bit difficult to see :-)

I have split this into smaller patches, the first patch adding SVE2 feature support can be found here:
https://reviews.llvm.org/D61513

First couple of patches adding instructions:
https://reviews.llvm.org/D61514
https://reviews.llvm.org/D61515

c-rhodes marked an inline comment as done.May 3 2019, 8:45 AM
c-rhodes added inline comments.
lib/Target/AArch64/AArch64SchedExynosM1.td
27

@javed.absar I've split this patch up and implemented your suggestion in this revision: https://reviews.llvm.org/D61513

c-rhodes abandoned this revision.Jun 3 2019, 3:42 AM

This was split up into smaller patches, all have now been merged. Abandoning this revision.