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

[Bradley Smith]

I've fixed all of these up and committed as r193859, thanks.

Regards,
Bradley Smith

> -----Original Message-----
> From: Kristof Beyls
> Sent: 01 November 2013 12:39
> To: reviews+D2078+public+348a20492ceb2ad4 at llvm-reviews.chandlerc.com;
> 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.
> 
> 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