[AArch64] Restore codegen for AArch64 Cortex-A72/A73 after NFCI

[sbaranga]

Summary

Code generation for Cortex-A72/Cortex-A73 was accidentally changed
by r271555, which was a NFCI. The isCortexA57() predicate was not true
for Cortex-A72/Cortex-A73 before r271555 (since it was checking the CPU
string). Because Cortex-A72/Cortex-A73 inherit all features from Cortex-A57,
all decisions previously guarded by isCortexA57() are now taken.

This change restores the behaviour before r271555 by adding separate
ProcA72/ProcA73, which have the required features to preserve code
generation.

[mcrosier]

Would it be possible to find a few codegen test cases that are specific to A57 and then add addtional RUN lines for A72 and A73 to make sure this doesn't regress?

[rengolin]

Ah, yes, that makes a lot of sense, and it's actually good to see A7x being more formally recognised.

The patch looks good to me, but I'm with Chad that we could have at least some tests to show the behaviour...

[sbaranga]

In D21182#453385, @mcrosier wrote:

Would it be possible to find a few codegen test cases that are specific to A57 and then add addtional RUN lines for A72 and A73 to make sure this doesn't regress?

I suspect what we really want are tests for these features that aren't triggered by -mcpu=... (which is probably how they were initially added).

[rengolin]

In D21182#453412, @sbaranga wrote:

I suspect what we really want are tests for these features that aren't triggered by -mcpu=... (which is probably how they were initially added).

Can you add CPU features in command lines? Like:

-march=armv8a+FeaturePredictableSelectIsExpensive

If not, the only way to test those features is by selecting a CPU that has it versus one that hasn't.

[sbaranga]

In D21182#453416, @rengolin wrote:

In D21182#453412, @sbaranga wrote:

I suspect what we really want are tests for these features that aren't triggered by -mcpu=... (which is probably how they were initially added).

Can you add CPU features in command lines? Like:

-march=armv8a+FeaturePredictableSelectIsExpensive

If not, the only way to test those features is by selecting a CPU that has it versus one that hasn't.

They can be turned on/off with -mattr:

bin/llc ../llvm/test/Transforms/InstCombine/phi-preserve-ir-flags.ll -o - -mtriple=aarch64-linux-gnueabi -mattr=+merge-narrow-ld
bin/llc ../llvm/test/Transforms/InstCombine/phi-preserve-ir-flags.ll -o - -mtriple=aarch64-linux-gnueabi -mattr=-merge-narrow-ld

[rengolin]

In D21182#453431, @sbaranga wrote:

bin/llc ../llvm/test/Transforms/InstCombine/phi-preserve-ir-flags.ll -o - -mtriple=aarch64-linux-gnueabi -mattr=+merge-narrow-ld
bin/llc ../llvm/test/Transforms/InstCombine/phi-preserve-ir-flags.ll -o - -mtriple=aarch64-linux-gnueabi -mattr=-merge-narrow-ld

right, and if we already have those, then putting features into CPU descriptions don't need any additional test. It's also not going to regress, because we only do the migration once, and all future changes will be regarding features, not CPU names.

LGTM, assuming we already have all the necessary tests in place.

[mcrosier]

SGTM.

[sbaranga]

Rebase this change against ToT.

[sbaranga]

Committed in r273277.

Thanks,
Silviu

lib/Target/AArch64/AArch64.td

def ProcA57     : SubtargetFeature<"a57", "ARMProcFamily", "CortexA57",
                                    FeatureFPARMv8,
                                    FeatureMergeNarrowLd,
                                    FeatureNEON,
                                    FeaturePerfMon,
                                    FeaturePostRAScheduler,
                                    FeaturePredictableSelectIsExpensive
                                    ]>;
 
+def ProcA72     : SubtargetFeature<"a72", "ARMProcFamily", "CortexA72",
+                                   "Cortex-A72 ARM processors", [
+                                   FeatureCRC,
+                                   FeatureCrypto,
+                                   FeatureFPARMv8,
+                                   FeatureNEON,
+                                   FeaturePerfMon
+                                   ]>;
+
+def ProcA73     : SubtargetFeature<"a73", "ARMProcFamily", "CortexA73",
+                                   "Cortex-A73 ARM processors", [
+                                   FeatureCRC,
+                                   FeatureCrypto,
+                                   FeatureFPARMv8,
+                                   FeatureNEON,
+                                   FeaturePerfMon
+                                   ]>;
+
 def ProcCyclone : SubtargetFeature<"cyclone", "ARMProcFamily", "Cyclone",
                                    "Cyclone", [
                                    FeatureAlternateSExtLoadCVTF32Pattern,
                                    FeatureCrypto,
                                    FeatureDisableLatencySchedHeuristic,
                                    FeatureFPARMv8,
                                    FeatureMacroOpFusion,
                                    FeatureNEON,
def : ProcessorModel<"generic", NoSchedModel, [
                      FeaturePostRAScheduler
                      ]>;
 
 // FIXME: Cortex-A35 is currently modelled as a Cortex-A53
 def : ProcessorModel<"cortex-a35", CortexA53Model, [ProcA35]>;
 def : ProcessorModel<"cortex-a53", CortexA53Model, [ProcA53]>;
 def : ProcessorModel<"cortex-a57", CortexA57Model, [ProcA57]>;
 // FIXME: Cortex-A72 and Cortex-A73 are currently modelled as an Cortex-A57.
-def : ProcessorModel<"cortex-a72", CortexA57Model, [ProcA57]>;
-def : ProcessorModel<"cortex-a73", CortexA57Model, [ProcA57]>;
+def : ProcessorModel<"cortex-a72", CortexA57Model, [ProcA72]>;
+def : ProcessorModel<"cortex-a73", CortexA57Model, [ProcA73]>;
 def : ProcessorModel<"cyclone", CycloneModel, [ProcCyclone]>;
 def : ProcessorModel<"exynos-m1", ExynosM1Model, [ProcExynosM1]>;
 def : ProcessorModel<"kryo", KryoModel, [ProcKryo]>;
 def : ProcessorModel<"vulcan", NoSchedModel, [ProcVulcan]>;
 
 //===----------------------------------------------------------------------===//
 // Assembly parser
 //===----------------------------------------------------------------------===//

lib/Target/AArch64/AArch64Subtarget.h

 
 class AArch64Subtarget : public AArch64GenSubtargetInfo {
 public:
   enum ARMProcFamilyEnum : uint8_t {
     Others,
     CortexA35,
     CortexA53,
     CortexA57,
+    CortexA72,
+    CortexA73,
     Cyclone,
     ExynosM1,
     Kryo,
     Vulcan
   };
 
 protected:
   /// ARMProcFamily - ARM processor family: Cortex-A53, Cortex-A57, and others.

lib/Target/AArch64/AArch64Subtarget.cpp

case ExynosM1:
     break;
   case Kryo:
     MaxInterleaveFactor = 4;
     VectorInsertExtractBaseCost = 2;
     break;
   case Vulcan: break;
   case CortexA35: break;
   case CortexA53: break;
+  case CortexA72: break;
+  case CortexA73: break;
   case Others: break;
   }
 }
 
 AArch64Subtarget::AArch64Subtarget(const Triple &TT, const std::string &CPU,
                                    const std::string &FS,
                                    const TargetMachine &TM, bool LittleEndian)
     : AArch64GenSubtargetInfo(TT, CPU, FS), ReserveX18(TT.isOSDarwin()),