[ARM] Correct ARMv8*-A optional extension definitions in TargetParser

[richard.barton.arm]

Summary

The ARMv8*-A descriptions in the ARM and AArch64 TargetParsersa are incorrect
architecturally and mismatched to the backend descriptions.

RAS is an optional extension to ARMv8-A and ARMv8.1-A and mandatory in
ARMv8.2-A. Correct the ARMTargetParser descriptions which had this as enabled
by default in the earlier versions.

The FP16 and SPE extensions are optional in ARMv8.2-A and the backend defaults
them as off. They are not available as extensions to earlier ARMv8-A versions.
Correct the AArch64TargetParser which had these as enabled by default in all
ARMv8-A definitions.

These macros are only used to define preprocessor macros. There are no macros
yet as ACLE has not caught up with ARMv8.2-A so not possible to add a test.

[t.p.northover]

Looks reasonable to me, but we ought to be able to add MC tests (or even unittests) to make sure it sticks.

[rengolin]

Shouldn't be hard to add unit tests...

[jojo]

I have checked the TargetParser and related unittests, the change is reasonable and enough.
LGTM!

[t.p.northover]

JoJo: two of us have asked for tests. Are you seeing something we're not?

[jojo]

Hi Tim,

I have checked the TargetParser. no addtional tests are needed for TargetParser as i see.
Please let me know if you find something that I may missed. :)

[rengolin]

Hi Tim,

Some of the TargetParser tests are automatic, with both the tests and the checks generated from the same tables, so they will continue to stress those lines.

The validity of this technique may be discussed, and tests changed, but I don't think it should be related to this commit.

cheers,
--renato

[t.p.northover]

Ah, I see. Didn't realise that.

[richard.barton.arm]

Hi all - I was puzzling over how to write a new unit test that would catch this error without just copying the structure of the target parser itself. The best I could come up with is to define a for each architecture extension, a set of all architectures that must enable it by default. You could then iterate over all the extensions and for each check all the architectures and test that those in the set have the enum set and those not in the set do not.

So like:

const char *RASDefaultArches[] = { "armv8.2-a"};
..
const struct {AArch64::ArchExtKind Ext, const char *DefaultArches[] } Defaults[] =  {
    {AArch64::AEK_RAS, RASDefaultArches },
    ...
};
for D in Defaults:
  for Ext in D.first():
    for Arch in AllArches:
      ASSERT(Arch in D.second() ? Arch.ArchBaseExtensions & Ext : !(Arch.ArchBaseExtensions & Ext))

Does seem a little contrived though. If you all are happy with the existing testing then I will land the patch as-is. Happy to work on further tests if they are needed. When the ACLE catches up then we can test these properly I think.

[rengolin]

In D23500#519404, @richard.barton.arm wrote:

const char *RASDefaultArches[] = { "armv8.2-a"};
..
const struct {AArch64::ArchExtKind Ext, const char *DefaultArches[] } Defaults[] = {

{AArch64::AEK_RAS, RASDefaultArches },
...

};
for D in Defaults:

for Ext in D.first():
  for Arch in AllArches:
    ASSERT(Arch in D.second() ? Arch.ArchBaseExtensions & Ext : !(Arch.ArchBaseExtensions & Ext))

Does seem a little contrived though. If you all are happy with the existing testing then I will land the patch as-is. Happy to work on further tests if they are needed. When the ACLE catches up then we can test these properly I think.

This would be the pattern that I was expecting, yes, but for all arches / extensions.

My problem with that technique is that new arches / extensions won't fail the tests, so may get away with not adding tests, but I don't have a good strategy to make it break if a new extension is added... Maybe this could be two tests, or a complicated one...

I'm open to suggestions. I also don't mind this commit going as is and you adding some more generic tests later.

But I think these tests will need reviewing one way or another.

llvm/trunk/include/llvm/Support/AArch64TargetParser.def

 #ifndef AARCH64_ARCH
 #define AARCH64_ARCH(NAME, ID, CPU_ATTR, SUB_ARCH, ARCH_ATTR, ARCH_FPU, ARCH_BASE_EXT)
 #endif
 AARCH64_ARCH("invalid", AK_INVALID, nullptr, nullptr,
              ARMBuildAttrs::CPUArch::v8_A, FK_NONE, AArch64::AEK_NONE)
 AARCH64_ARCH("armv8-a", AK_ARMV8A, "8-A", "v8", ARMBuildAttrs::CPUArch::v8_A,
              FK_CRYPTO_NEON_FP_ARMV8,
              (AArch64::AEK_CRC | AArch64::AEK_CRYPTO | AArch64::AEK_FP |
-              AArch64::AEK_SIMD | AArch64::AEK_FP16 | AArch64::AEK_PROFILE))
+              AArch64::AEK_SIMD))
 AARCH64_ARCH("armv8.1-a", AK_ARMV8_1A, "8.1-A", "v8.1a",
              ARMBuildAttrs::CPUArch::v8_A, FK_CRYPTO_NEON_FP_ARMV8,
              (AArch64::AEK_CRC | AArch64::AEK_CRYPTO | AArch64::AEK_FP |
-              AArch64::AEK_SIMD | AArch64::AEK_FP16 | AArch64::AEK_PROFILE))
+              AArch64::AEK_SIMD))
 AARCH64_ARCH("armv8.2-a", AK_ARMV8_2A, "8.2-A", "v8.2a",
              ARMBuildAttrs::CPUArch::v8_A, FK_CRYPTO_NEON_FP_ARMV8,
              (AArch64::AEK_CRC | AArch64::AEK_CRYPTO | AArch64::AEK_FP |
-              AArch64::AEK_SIMD | AArch64::AEK_FP16 | AArch64::AEK_PROFILE |
-              AArch64::AEK_RAS))
+              AArch64::AEK_SIMD | AArch64::AEK_RAS))
 #undef AARCH64_ARCH
 
 #ifndef AARCH64_ARCH_EXT_NAME
 #define AARCH64_ARCH_EXT_NAME(NAME, ID, FEATURE, NEGFEATURE)
 #endif
 // FIXME: This would be nicer were it tablegen
 AARCH64_ARCH_EXT_NAME("invalid",  AArch64::AEK_INVALID,  nullptr,  nullptr)
 AARCH64_ARCH_EXT_NAME("none",     AArch64::AEK_NONE,     nullptr,  nullptr)

llvm/trunk/include/llvm/Support/ARMTargetParser.def

ARM_ARCH("armv7e-m", AK_ARMV7EM, "7E-M", "v7em", ARMBuildAttrs::CPUArch::v7E_M,
           FK_NONE, (ARM::AEK_HWDIV | ARM::AEK_DSP))
 ARM_ARCH("armv8-a", AK_ARMV8A, "8-A", "v8", ARMBuildAttrs::CPUArch::v8_A,
          FK_CRYPTO_NEON_FP_ARMV8,
          (ARM::AEK_SEC | ARM::AEK_MP | ARM::AEK_VIRT | ARM::AEK_HWDIVARM |
           ARM::AEK_HWDIV | ARM::AEK_DSP | ARM::AEK_CRC))
 ARM_ARCH("armv8.1-a", AK_ARMV8_1A, "8.1-A", "v8.1a",
          ARMBuildAttrs::CPUArch::v8_A, FK_CRYPTO_NEON_FP_ARMV8,
          (ARM::AEK_SEC | ARM::AEK_MP | ARM::AEK_VIRT | ARM::AEK_HWDIVARM |
-          ARM::AEK_HWDIV | ARM::AEK_DSP | ARM::AEK_CRC | ARM::AEK_RAS))
+          ARM::AEK_HWDIV | ARM::AEK_DSP | ARM::AEK_CRC))
 ARM_ARCH("armv8.2-a", AK_ARMV8_2A, "8.2-A", "v8.2a",
          ARMBuildAttrs::CPUArch::v8_A, FK_CRYPTO_NEON_FP_ARMV8,
          (ARM::AEK_SEC | ARM::AEK_MP | ARM::AEK_VIRT | ARM::AEK_HWDIVARM |
-          ARM::AEK_HWDIV | ARM::AEK_DSP | ARM::AEK_CRC))
+          ARM::AEK_HWDIV | ARM::AEK_DSP | ARM::AEK_CRC | ARM::AEK_RAS))
 ARM_ARCH("armv8-m.base", AK_ARMV8MBaseline, "8-M.Baseline", "v8m.base",
           ARMBuildAttrs::CPUArch::v8_M_Base, FK_NONE, ARM::AEK_HWDIV)
 ARM_ARCH("armv8-m.main", AK_ARMV8MMainline, "8-M.Mainline", "v8m.main",
           ARMBuildAttrs::CPUArch::v8_M_Main, FK_FPV5_D16, ARM::AEK_HWDIV)
 // Non-standard Arch names.
 ARM_ARCH("iwmmxt", AK_IWMMXT, "iwmmxt", "", ARMBuildAttrs::CPUArch::v5TE,
           FK_NONE, ARM::AEK_NONE)
 ARM_ARCH("iwmmxt2", AK_IWMMXT2, "iwmmxt2", "", ARMBuildAttrs::CPUArch::v5TE,