[PATCH] D21432: [ARM] Do not test for CPUs, use SubtargetFeatures (Part 1). NFC

[Diana Picus via llvm-commits]

rovka added a comment.

Thanks for having a look.


================
Comment at: lib/Target/ARM/ARM.td:111
@@ +110,3 @@
+// during if conversion.
+def FeatureProfUnpredicate : SubtargetFeature<"prof-unpr",
+                                              "IsProfitableToUnpredicate",
----------------
compnerd wrote:
> I think that it may be easier to actually have this be inverted and have a "cheap predicates" feature which would normally be false.
This takes its name from TargetInstrInfo::IsProfitableToUnpredicate, which is where it's used. I'm not sure it's worth the trouble of renaming that for all the targets.

================
Comment at: lib/Target/ARM/ARM.td:132
@@ +131,3 @@
+// than ISH
+def FeaturePrefISHSTBarrier : SubtargetFeature<"pref-ishst", "PreferISHST",
+                                           "true", "Prefer ISHST barriers">;
----------------
compnerd wrote:
> Similar.
Did some spelling out, but I'd rather not spell it out in the feature name, as that would make it go over 80 chars in the features list (and I'd rather keep feature names short than realign the whole thing). There's a precedent for this in FeaturePref32BitThumb.

================
Comment at: lib/Target/ARM/ARM.td:137
@@ +136,3 @@
+// VFP to NEON.
+def FeatureNEONForFPMovs : SubtargetFeature<"neon-fpmovs", "UseNEONForFPMovs",
+                              "true", "Convert VMOVSR, VMOVRS, VMOVS to NEON">;
----------------
compnerd wrote:
> `FeatureNEONFPMoves` perhaps?
There's already a FeatureNEONForFP, I followed that one (although I'm not against renaming this or both of them).

While we're at it: I find it a bit peculiar that FeatureNEONForFP is enabled only for Swift and Cyclone, and FeatureNEONForFPMoves is enabled only for Cortex-A9. I'm not sure if this is an oversight or if I just picked a really bad name for this feature. Any thoughts?

================
Comment at: lib/Target/ARM/ARMSubtarget.h:221-237
@@ -220,1 +220,19 @@
 
+  /// If true, if conversion may decide to leave some instructions unpredicated.
+  bool IsProfitableToUnpredicate;
+
+  /// If true, VMOV will be favored over VGETLNi32.
+  bool HasSlowVGETLNi32;
+
+  /// If true, VMOV will be favored over VDUP.
+  bool HasSlowVDUP32;
+
+  /// If true, VMOVSR will be favored over VMOVDRR.
+  bool PreferVMOVSR;
+
+  /// If true, ISHST barriers will be used for Release semantics.
+  bool PreferISHST;
+
+  /// If true, VMOVRS, VMOVSR and VMOVS will be converted from VFP to NEON.
+  bool UseNEONForFPMovs;
+
----------------
MatzeB wrote:
> This needs to be initialized, you may want to do a similar commit as I did in r271057 and use member initializers throughout the Subtarget class.
Oops... Looks like for ARM this is done in ARMSubtarget::initializeEnvironment, so I added it there.


http://reviews.llvm.org/D21432