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

[Diogo N. Sampaio via Phabricator via llvm-commits]

dnsampaio marked 2 inline comments as done.
dnsampaio added inline comments.


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


================
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
----------------
DavidSpickett wrote:
> (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.
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`.


================
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
----------------
DavidSpickett wrote:
> 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)
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



Repository:
  rL LLVM

https://reviews.llvm.org/D54633