[PATCH] D20762: AArch64: Do not test for CPUs, use SubtargetFeatures

[Renato Golin via llvm-commits]

rengolin added a comment.

Hi Matthias,

I think this is a really good change to make, but I have some comments in addition to Diana...

cheers,
--renato


================
Comment at: lib/Target/AArch64/AArch64.td:76
@@ +75,3 @@
+
+def FeatureInterleaveFactor4 : SubtargetFeature<"interleave-factor-4",
+    "MaxInterleaveFactor", "4",
----------------
Can't this be a property?

================
Comment at: lib/Target/AArch64/AArch64.td:102
@@ +101,3 @@
+
+def FeatureVectorInsertExtractCost2 : SubtargetFeature<
+    "vector-insert-extract-cost-2", "VectorInsertExtractBaseCost", "2",
----------------
This sounds like an odd one.

================
Comment at: lib/Target/AArch64/AArch64Subtarget.cpp:153
@@ -140,6 +152,3 @@
 AArch64Subtarget::getCustomPBQPConstraints() const {
-  if (!isCortexA57())
-    return nullptr;
-
-  return llvm::make_unique<A57ChainingConstraint>();
+  return balanceFPOps() ? llvm::make_unique<A57ChainingConstraint>() : nullptr;
 }
----------------
What does load balancing FP have to do with PBQP?

Also, this moves from just A57 to both A57 and A53.

================
Comment at: lib/Target/AArch64/AArch64Subtarget.h:36
@@ -35,3 +35,3 @@
 class AArch64Subtarget : public AArch64GenSubtargetInfo {
-protected:
-  enum ARMProcFamilyEnum {
+public:
+  enum ARMProcFamilyEnum : uint8_t {
----------------
I'm not convinced that this will discourage people to avoid CPU checking more than having the isCPU methods... But it also won't encourage.

================
Comment at: lib/Target/AArch64/AArch64Subtarget.h:49
@@ +48,3 @@
+    NoSoftwarePrefetch,
+    SoftwarePrefetchCyclone,
+  };
----------------
Since we only have one, maybe try to not use the CPU name in it? Otherwise, we'd encourage things like "CycloneLike", which aren't much of an advantage of isCyclone.


Repository:
  rL LLVM

http://reviews.llvm.org/D20762