[PATCH] [ARM] Add Virtualization subtarget feature and add more build attributes in this area.

[Kristof Beyls]

Hi Bradley,

Just a few minor remarks on the patch:

 def ProcA53     : SubtargetFeature<"a53", "ARMProcFamily", "CortexA53",
                                    "Cortex-A53 ARM processors",
                                    [FeatureMP, FeatureHWDiv, FeatureHWDivARM,
                                     FeatureTrustZone, FeatureT2XtPk,
-                                    FeatureCrypto, FeatureCRC]>;
+                                    FeatureCrypto, FeatureCRC,
+                                    FeatureVirtualization]>;
 
 def ProcA57     : SubtargetFeature<"a57", "ARMProcFamily", "CortexA57",
                                    "Cortex-A57 ARM processors",
                                    [FeatureMP, FeatureHWDiv, FeatureHWDivARM,
                                     FeatureTrustZone, FeatureT2XtPk,
-                                    FeatureCrypto, FeatureCRC]>;
+                                    FeatureCrypto, FeatureCRC,
+                                    FeatureVirtualization]>;

The ARMARM states that the Virtualization and the Multiprocessing extensions are
always part of the v8 A32 and T32 instruction sets. Therefore, it would be best
not to add FeatureVirtualization directly to cores implementing the v8
architecture, but instead add FeatureVirtualization to be implied by HasV8Ops.
For the same reason, FeatureMP should also be removed from A53/A57 and moved to
HasV8Ops.


+  /// HasVirtualization - True if the subtarget supports Virtualzation
+  bool HasVirtualization;
+

There's a typo in the comment. Maybe it's better to say "... if the
subtarget supports the Virtualization extension".


+; CORTEX-A9-NOT:  .eabi_attribute 42
+; CORTEX-A9:  .eabi_attribute 68, 1

I think a RUN line must be added for cortex-a9-mp to verify that the
Tag_MPextension_use attribute gets set to value 1?


With the above changes, the patch looks good to me and can be committed.


Thanks,

Kristof

> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Bradley Smith
> Sent: 31 October 2013 17:32
> To: Bradley Smith; t.p.northover at gmail.com
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH] [ARM] Add Virtualization subtarget feature and add more
> build attributes in this area.
> 
>   Rework test/CodeGen/ARM/2010-10-19-mc-elf-objheader.ll testcase to something
> that is more maintainable. This changes the focus of this testcase away from
> testing CPU defaults (which is tested elsewhere), onto specifically testing
> that attributes are encoded correctly.
> 
> http://llvm-reviews.chandlerc.com/D2078
> 
> CHANGE SINCE LAST DIFF
>   http://llvm-reviews.chandlerc.com/D2078?vs=5289&id=5296#toc
> 
> Files:
>   lib/Target/ARM/ARM.td
>   lib/Target/ARM/ARMAsmPrinter.cpp
>   lib/Target/ARM/ARMBuildAttrs.h
>   lib/Target/ARM/ARMSubtarget.cpp
>   lib/Target/ARM/ARMSubtarget.h
>   test/CodeGen/ARM/2010-09-29-mc-asm-header-test.ll
>   test/CodeGen/ARM/2010-10-19-mc-elf-objheader.ll
>   test/CodeGen/ARM/build-attributes-encoding.s