[PATCH] D54633: [NFC][AArch64] Split out backend features

[David Spickett via Phabricator via llvm-commits]

DavidSpickett added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:46
+
+def HasRASv8_4       : Predicate<"Subtarget->hasRASv8_4()">,
+                       AssemblerPredicate<"FeatureRASv8_4", "rasV8.4a">;
----------------
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.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:2677
                                   (am_unscaled8 GPR64sp:$Rn, simm9:$offset))]>;
-
-// Armv8.4 LDAPR & STLR with Immediate Offset instruction
-let Predicates = [HasV8_4a] in {
+// Armv8.4 Weaker Release Consistency enhancements
+//         LDAPR & STLR with Immediate Offset instructions
----------------
Nitpick: add a blank line here.


================
Comment at: test/MC/AArch64/armv8.2a-persistent-memory.s:7
 // CHECK: dc cvap, x7   // encoding: [0x27,0x7c,0x0b,0xd5]
-// ERROR: error: DC CVAP requires ARMv8.2a
+// ERROR: error: DC CVAP requires ccpp
----------------
(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.


================
Comment at: test/MC/AArch64/armv8.3a-signed-pointer.s:142
+// CHECK-NEXT: pacia x0, x1     // encoding: [0x20,0x00,0xc1,0xda]
+// CHECK-REQ-NEXT:      ^
+// CHECK-REQ-NEXT: error: instruction requires: pa
----------------
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)


Repository:
  rL LLVM

https://reviews.llvm.org/D54633