[PATCH] D25211: [ARM] Add Cortex-R52 target to LLVM

[James Molloy via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added inline comments.
This revision now requires changes to proceed.


> ARMTargetParser.def:98
> +ARM_ARCH("armv8-r", AK_ARMV8R, "8-R", "v8r", ARMBuildAttrs::CPUArch::v8_R,
> +          FK_NEON_FP_ARMV8, (ARM::AEK_SEC | ARM::AEK_MP | ARM::AEK_VIRT | ARM::AEK_HWDIVARM | ARM::AEK_HWDIV |
> +                             ARM::AEK_DSP | ARM::AEK_CRC))

OK, it took me a while to get to the bottom of this list and I had to grep through a lot of pre-alpha unreleased documentation so I'll try to explain my reasoning:

- AEK_SEC - *INCORRECT* - I think this means TrustZone and ARMv8-R does not have TrustZone.
- AEK_MP - Multiprocessing extensions - Mandatory
- AEK_VIRT - Virtualization extensions - My reading is that this is mandatory although I can't find that definitively stated anywhere as yet.
- AEK_HWDIVARM | AEK_HWDIV - UDIV/SDIV in A32/T32 modes - These are transitively manadatory because of AEK_VIRT (see ARMv7AR ARMARM 4.4.4 "ARMv7 implementation requirements and options for the divide instructions")
- AEK_DSP - Required in ARMv8-A, and not explicitly mentioned otherwise.
- AEK_CRC - Required.

> ARMTargetParser.def:226
>               (ARM::AEK_MP | ARM::AEK_HWDIVARM))
> +ARM_CPU_NAME("cortex-r52", AK_ARMV8R, FK_NEON_FP_ARMV8, false, ARM::AEK_CRC)
>  ARM_CPU_NAME("sc300", AK_ARMV7M, FK_NONE, false, ARM::AEK_NONE)

I don't think you should mention CRC here because it's no longer an optional extension, it's mandatory.

> ARM.td:482
> +                                                       FeatureDB,
> +                                                       FeatureT2XtPk,
> +                                                       FeatureDSP,

I agree with all of these except this. Why does ARMv8-R have this but ARMv8-A does not?

> ARM.td:820
>  
> +def : ProcNoItin<"cortex-r52",                          [ARMv8r, ProcR52,
> +                                                         FeatureHWDiv,

Because ARMv8-R mandates HWDiv and HWDivARM, I don't think these are required here.

https://reviews.llvm.org/D25211