[PATCH] D113256: [AArch64][ARM] Enablement of Cortex-A710 Support

[Dave Green via Phabricator via cfe-commits]

dmgreen added inline comments.


================
Comment at: llvm/include/llvm/Support/AArch64TargetParser.def:181
+                  AArch64::AEK_SVE2BITPERM | AArch64::AEK_BF16))
 AARCH64_CPU_NAME("cortex-a78c", ARMV8_2A, FK_CRYPTO_NEON_FP_ARMV8, false,
                  (AArch64::AEK_FP16 | AArch64::AEK_DOTPROD | AArch64::AEK_RCPC |
----------------
I would go after this, it being a a78-like cpu it's probably worth keeping the two together.


================
Comment at: llvm/include/llvm/Support/AArch64TargetParser.def:159-160
                   AArch64::AEK_RCPC | AArch64::AEK_SSBS))
+AARCH64_CPU_NAME("cortex-a710", ARMV9A, FK_NEON_FP_ARMV8, false,
+                 (AArch64::AEK_MTE | AArch64::AEK_PAUTH | AArch64::AEK_FLAGM |
+                  AArch64::AEK_SB | AArch64::AEK_I8MM | AArch64::AEK_FP16FML |
----------------
dmgreen wrote:
> lenary wrote:
> > dmgreen wrote:
> > > Natural order would be better I think, where the new A710 is added after the A78.
> > > 
> > > FlagM should already be included as a part of 8.4, so isn't needed here.
> > > Should BFloat16 be added?
> > > FlagM should already be included as a part of 8.4, so isn't needed here.
> > 
> > FlagM is not part of the `AARCH64_ARCH("armv9-a", ARMV9A ...)`  definition, nor part of the equivalents for armv8.4a and armv8.5a, so it was added explicitly here.
> > 
> > > Should BFloat16 be added?
> > 
> > Yes
> > 
> > 
> > 
> FeatureFlagM is included in HasV8_4aOps. So should already be included (much like something like dotprod).
> 
> Why AEK_FLAGM isn't part of ARMV8_4A I don't know. And why we have two maps for the same set of information...
I think that AEK_FLAGM should be included in architecture that is 8.4+. Then it shouldn't be needed here.
Perhaps lets do that as a separate commit though.


================
Comment at: llvm/include/llvm/Support/ARMTargetParser.def:315
+              ARM::AEK_I8MM))
 ARM_CPU_NAME("cortex-a78c", ARMV8_2A, FK_CRYPTO_NEON_FP_ARMV8, false,
              ARM::AEK_FP16 | ARM::AEK_DOTPROD)
----------------
Ditto and same in other places.


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:87
     PrefFunctionLogAlignment = 4;
-    VScaleForTuning = 1;
     break;
----------------
Make sure you keep this in. It looks like it might have been removed by accident.


================
Comment at: llvm/unittests/Support/TargetParserTest.cpp:278
                          "8-A"),
+        ARMCPUTestParams("cortex-a710", "armv9-a", "neon-fp-armv8",
+                         ARM::AEK_SEC | ARM::AEK_MP | ARM::AEK_VIRT |
----------------
Ordering.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113256/new/

https://reviews.llvm.org/D113256