Page MenuHomePhabricator

[AArch64][SVE2] Add SVE2 target features to backend and TargetParser
ClosedPublic

Authored by c-rhodes on May 3 2019, 8:19 AM.

Diff Detail

Event Timeline

c-rhodes created this revision.May 3 2019, 8:19 AM
c-rhodes edited the summary of this revision. (Show Details)May 3 2019, 8:21 AM
SjoerdMeijer added inline comments.May 3 2019, 9:09 AM
unittests/Support/TargetParserTest.cpp
1018

Please upload diffs with more context (-U999999). I can't see if you're actually adding tests here.

rovka added a subscriber: rovka.May 6 2019, 2:12 AM
rovka added inline comments.
lib/Support/AArch64TargetParser.cpp
100

It might be a bit long, but I'd prefer sve2-bitperm here, to avoid the kind of situation we have in the ARM backend with hwdiv / hwdiv-arm.

lib/Target/AArch64/AArch64InstrInfo.td
118

This should be Subtarget->hasSVE2BitPerm(), right?

lib/Target/AArch64/AArch64Subtarget.h
142

Wouldn't it be safer to default to false? Otherwise you'd always have to check hasSVE2() first, which is a bit of a chore.

c-rhodes added inline comments.May 7 2019, 4:45 AM
lib/Support/AArch64TargetParser.cpp
100

It was decided to name this bitperm to avoid the situation where any potential future SVE2 extensions will be enabled with sve2-xxx as this will make the -march longer and so far architecture features have not been prefixed with arch level or vector extension name.

I agree with your concern regarding the hwdiv / hwdiv-arm, I will raise this but it's worth mentioning it's not trivial to change at this point as Binutils are moving forward with the same flag names.

lib/Target/AArch64/AArch64InstrInfo.td
118

Yeah, good spot!

lib/Target/AArch64/AArch64Subtarget.h
142

That does seem logical, this is following the Armv8.4 Crypto extensions defined in this file which can't be seen in this diff because I didn't use a big enough context (about to upload full context). Patch adding those features can be seen here: https://reviews.llvm.org/D48625#change-H8SQOdLbC5nS

@SjoerdMeijer what was the reasoning for setting the default as true for the v8.4 crypto extensions?

c-rhodes updated this revision to Diff 198437.May 7 2019, 4:46 AM

Uploaded patch with more context. No actual changes since last revision.

c-rhodes marked an inline comment as done.May 7 2019, 4:47 AM
SjoerdMeijer added inline comments.May 8 2019, 12:44 AM
lib/Target/AArch64/AArch64Subtarget.h
142

Good question. I can't remember now. I can only think of the excuse that crypto got changed and also backported (the comment is wrong too because they are optional extensions to Armv8.2), but am not sure it's a valid excuse. I will look into this.

SjoerdMeijer accepted this revision.May 8 2019, 12:54 AM

With Diana's comments to about Subtarget->hasSVE2BitPerm() and defaulting the booleans to false fixed, this looks okay to me.

Please wait a day or so before committing just in case there are more comments; I think these fixes are trivial, no need to upload a new diff here.

lib/Support/AArch64TargetParser.cpp
100

I do see Diana's point, but if this was synchronised with the GNU community, and there isn't really a precedent for prefixing it, then this seems okay to me.

This revision is now accepted and ready to land.May 8 2019, 12:54 AM
rovka accepted this revision.May 8 2019, 1:05 AM

LGTM also with the remaining comments addressed.

lib/Support/AArch64TargetParser.cpp
100

I agree, let's leave it as is.

Great, I'll merge tomorrow with the minor changes then. Thanks for reviewing!

This revision was automatically updated to reflect the committed changes.