Page MenuHomePhabricator

[NFC][AArch64] Split out backend features
ClosedPublic

Authored by dnsampaio on Nov 16 2018, 8:41 AM.

Details

Summary

This patch splits backend features currently hidden behind architecture versions.
For example, currently the only way to activate complex numbers extension is targeting an v8.3 architecture, where after the patch this extension can be added separately.
This refactoring is required by the new command lines proposal:
http://lists.llvm.org/pipermail/llvm-dev/2018-September/126346.html

Diff Detail

Event Timeline

dnsampaio created this revision.Nov 16 2018, 8:41 AM
DavidSpickett added inline comments.Mon, Nov 19, 1:38 AM
lib/Target/AArch64/AArch64InstrInfo.td
46

Can you clarify what RASv8_4 adds? Is it a 'crypto' situation where the v8_4 meaning of crypto includes the previous meaning of crypto?

I'm wondering if these instructions are better described as requiring v8.4 and RAS, but I'd need to check the spec to get a better idea. If what it adds is completely separate then a new feature is probably fine.

2680–2682

Nitpick: add a blank line here.

test/MC/AArch64/armv8.2a-persistent-memory.s
7

(just picked a random test line, it's not important which one)

What's our usual policy where an instruction could be enabled by a feature or an arch? Do we currently say:
instr requires: one of armv8.x, feature
Or, as this line does:
instr requires feature

I'm not bothered about adding that information if we don't already do it, but we should follow previous precedent.

test/MC/AArch64/armv8.3a-signed-pointer.s
142

I think these lines are out of order, it may be ignoring this first line. (and it's the only '^' in this file so it sticks out)

Also, is this really NFC? It should be no change to existing usage, but it adds new options so that's a change at least to llvm-mc.

dnsampaio marked 2 inline comments as done.Mon, Nov 19, 3:30 AM
dnsampaio added inline comments.
lib/Target/AArch64/AArch64InstrInfo.td
46

RAS v8.4 imposes less constraints of use of the barrier (ESB) use, from RAS (v8.2). It also add new features. So it is sort of a 'crypto' thing.

However, I would prefer to split the feature into rasv8_4, and declare that it implies the older RAS. Keeping a distinct feature makes it easier to backported it to ARM v8.3, if ever required.

test/MC/AArch64/armv8.2a-persistent-memory.s
7

That comes directly from tablegen, and it goes with the name of the feature that it requires. Unfortunately, tablegen won't allow to say it requires ccpp OR armv8.x.

I relied on the test of dotprod, where it says only requires dotprod.

test/MC/AArch64/armv8.3a-signed-pointer.s
142

Not really. I'm ensuring that in the file with the missing requirements, there is no other line between
msr apgakeyhi_el1, x0
and
pacia x0, x1

using CHECK-REQ-NEXT:

CHECK-REQ: error: expected writable system register or pstate
CHECK-REQ-NEXT: msr apgakeyhi_el1, x0
CHECK-REQ-NEXT: ^
CHECK-REQ-NEXT: error: instruction requires: pa
// CHECK-REQ-NEXT: pacia x0, x1

dnsampaio updated this revision to Diff 174585.Mon, Nov 19, 3:47 AM
dnsampaio marked an inline comment as done.

Added FeatureRASv8_4 implies RAS.
NIT fix.

bryanpkc added inline comments.Thu, Nov 22, 10:45 PM
lib/Target/AArch64/AArch64.td
88

Shouldn't this feature imply FeatureID_ISAR6_EL1 instead of FeatureID_AA64MMFR2_EL1?

220

Shouldn't this feature imply FeatureID_ISAR6_EL1 instead of FeatureID_AA64MMFR2_EL1?

229

Shouldn't this feature imply FeatureID_ISAR6_EL1 instead of FeatureID_AA64MMFR2_EL1?

IMO, the description should say Javascript or JavaScript to make it easier to grep.

273

The architecture reference manual states that the ARMv8.4-TLBI feature is identified by ID_AA64ISAR0_EL1.TLB, not ID_AA64MMFR2_EL1. Is this discrepancy intentional?

Have you discussed these feature names with the GCC devs? I know we can change the user-facing names used by clang in in TargetParser, but it's easier for us if they match.

For the feature registers (ISAR6 and MMFR2), maybe we should just make these registers always available? These registers are all defined to read as all-zeroes in older architectures, which means no features described by that register are implemented. If we continue like this then we will have to add an extra feature to pretty much every new architectural feature for the ID register.

lib/Target/AArch64/AArch64.td
36

Why is this up here, and not with the other v8.2A features?

Hi @olista01 and @bryanpkc. Thanks for the reviews.

Have you discussed these feature names with the GCC devs? I know we can change the user-facing names used by clang in in TargetParser, but it's easier for us if they match.

Indeed I did not. As you said, they are not visible to the user. I'll check with the gcc devs how they have these features split and named though.

For the feature registers (ISAR6 and MMFR2), maybe we should just make these registers always available? These registers are all defined to read as all-zeroes in older architectures, which means no features described by that register are implemented. If we continue like this then we will have to add an extra feature to pretty much every new architectural feature for the ID register.

Indeed it seems the case for the ID_ISAR6_EL1 exist, so I'll just remove the constraints over it.
However, the ID_AA64MMFR2_EL1 register was added in v8.2. So I believe we should check as to warn the user if he might get undefined behavior by accessing it when not present.

lib/Target/AArch64/AArch64.td
36

I have not seen any pstates and registers Features before, so I created a region apart. No technical problem with it. I'll move UAO to the other features.

88

Indeed, ID_ISAR6_EL1 is used in arm32, not arm64. In arm64 it is identified using ID_AA64ISAR0_EL1, that has no constraints. So I can just remove this implication.

220

Yes, and same as the above, the implication is not required.

229

You are right with the naming. The space in Java Script was not intentional.
Nice catch again about the registers. Indeed for arm64 it is ID_AA64ISAR0_EL1.FHM using bits [51:48]. So I can just remove this implication.

273

Indeed the TLB instructions features are identified by ID_AA64ISAR0_EL1[59:56].
However, the use for TTL field in address operations is identified by ID_AA64MMFR2_EL1[51:48] field TTL. So this feature holds true. Even if the processor does not support it, it requires the register to tell it does not support it.

dnsampaio updated this revision to Diff 175279.Mon, Nov 26, 9:02 AM
dnsampaio marked 10 inline comments as done.

Fixed:
the features implications/dependencies.
the name of the JavaScript conversion feature.
some test files.

@olista01: Most GCC devs I asked about internal feature names emphatically said that sync of internal names of features is not required. And most of the features are not even visible internally, as they are also associated with architecture reviews. So not much to do about that.

So I tried to make their description as close as possible to the reference manual, as to make it easy to grep for them. Further changes for defining arguments should be straightforward.

bryanpkc added inline comments.Mon, Nov 26, 2:24 PM
lib/Target/AArch64/AArch64.td
283

Should FeatureWRC imply FeatureRCPC?

However, the ID_AA64MMFR2_EL1 register was added in v8.2. So I believe we should check as to warn the user if he might get undefined behavior by accessing it when not present.

The encoding of ID_AA64MMFR2_EL1 was defined to read as zero before Armv8.2A, so using it on earlier targets isn't a problem.

lib/Target/AArch64/AArch64.td
61

RAS doesn't use ID_AA64MMFR2_EL1.

93

None of the existing features have the with "has" in the name (first parameter), so I think we should stick with that convention.

231

This feature is in AA64MMFR2_EL1.

238

This should also depend on FeatureFullFP16.

252

Capitalisation: "PArtitioning"

259

I think this name is too generic, trace has always been a part of the 64-bit architecture, this feature just enables some extensions to it.

264

This should be "Activity", not "Active".

271

This name is too generic, maybe "tlbi-range"?

282

This is an extension over FeatureRCPC which adds immediate-offset versions of the instructions, so maybe FeatureRCPC/"rcpc-offset" would be better?

319

These are v8.2-A features, so they should be further up with the other v8.2A features.

lib/Target/AArch64/AArch64InstrInfo.td
2967

This is the "Limited ordering regions" (ARMv8.1-LOR in the ArmARM) extension, not "Virtualization host extensions" (ARMv8.1-VHE). This will need a separate subtarget feature.

lib/Target/AArch64/AArch64SystemOperands.td
67

Why does this depend on FeatureID_AA64MMFR2_EL1? This feature is identified by AA64MMFR1_EL1, not AA64MMFR2_EL1.

340

FeaturePsUAO already depends on FeatureID_AA64MMFR2_EL1, so I don't think we need to mention FeatureID_AA64MMFR2_EL1 here.

1291

This one should be FeaturePA, FeaturePAN is "Privileged Access Never".

dnsampaio marked 21 inline comments as done.Tue, Nov 27, 10:03 AM
dnsampaio added inline comments.
lib/Target/AArch64/AArch64.td
61

True.

93

Ok.

238

They FP16 instructions are dependent of FullFP16. In AArch64InstrFormats.td, you will see that such instructions depend on ComplNum, NEON and FullFP16.

259

Ok, will set to TRACEv8_4.

271

tlb-rmi TLB Range and Management Instructions?

283

Yes it should. Indeed the naming is not the best. Will set it to RCPC_IMMO, as for "RCPC with Immediate Offset".

lib/Target/AArch64/AArch64InstrInfo.td
2967

Indeed the feature is there, just got it wrong here. Thanks.

dnsampaio marked 5 inline comments as done.

Removed the register (system operand) constraint, and associated test case.
Fixed PAN, RCPC dependencies and instructions that depend on PA, not PAN.
Changed some names to be more meaningful.
Removed the prefix "has" from the features flags.
Fixed tests

Fix the tlb-rmi feature argument and test file.

NIT in the v8.4a trace tests.

dnsampaio updated this revision to Diff 175645.Wed, Nov 28, 3:30 AM

Fixed one comment.

olista01 accepted this revision.Mon, Dec 3, 1:54 AM

A few nits, but otherwise LGTM, no need to re-review after fixing them.

lib/Target/AArch64/AArch64.td
98

Feature name (first parameter) still contains "has".

231

Not done yet?

This revision is now accepted and ready to land.Mon, Dec 3, 1:54 AM
olista01 added inline comments.Mon, Dec 3, 2:41 AM
lib/Target/AArch64/AArch64.td
231

Ah, we decided to remove the subtarget feature for AA64MMFR2_EL1, ignore this.

This revision was automatically updated to reflect the committed changes.
dnsampaio updated this revision to Diff 176970.Thu, Dec 6, 7:19 AM

Trying to fix a windows bot failing test, dividing the test into multiple files and using full line matching.