[ARM] Reverse PostRASched subtarget feature logic

[samparker]

Summary

Replace the UsePostRAScheduler SubtargetFeature with DisablePostRAScheduler, which is then used by Swift and Cyclone. This patch maintains enabling PostRA scheduling for other Thumb2 capable cores and/or for functions which are being compiled in ARM mode.

[samparker]

Ping.

[fhahn]

Thanks Sam! I think this should be in line with Matthias's comments in a previous review, but I think it would be good if he could have another quick look.

lib/Target/ARM/ARM.td

879 ↗

(On Diff #112318)

Lots of unrelated whitespace changes. I think @javed.absar submitted a change recently to aligned all Processors model fields consistently.

lib/Target/ARM/ARMSubtarget.cpp

362 ↗

(On Diff #112318)

Convention seems to be using a "getter" functions to access properties.

364 ↗

(On Diff #112318)

Is PostRAScheduler still defined in some models? If so, I think we should remove those references, as they would be misleading IMO

[samparker]

Thanks Florian. I've removed my white space changes and introduced a customary getter function. I've also removed the PostRAScheduler flag from the Cortex-R52 model.

cheers,
sam

[javed.absar]

lib/Target/ARM/ARMSubtarget.cpp

364 ↗

(On Diff #113242)

may be add an explicit comment on why we choose to do this

[samparker]

lib/Target/ARM/ARMSubtarget.cpp

364 ↗

(On Diff #113242)

'Because the gods of old said so.' Honestly, I'm really not sure of the reason and the logic seems odd to me. Unfortunately a lot of tests rely on this behaviour and its not something that I want to look into at the moment.

[javed.absar]

lib/Target/ARM/ARMSubtarget.cpp

364 ↗

(On Diff #113242)

I think it is because of IT block - we don't want PostRA to mess it up.

[samparker]

lib/Target/ARM/ARMSubtarget.cpp

364 ↗

(On Diff #113242)

Ah, that makes sense, thanks!

[MatzeB]

Thanks for looking into this again. This is indeed better. Did you choose the "negative" DisablePostRAScheduler feature over a "positive" FeaturePostRAScheduler just so it's less lines needed in the .td file? The positive variant seems slightly user friendlier and in line with aarch64 to me, even though it means adding the flag to every processor variant except for swift and cyclone.

Either way this is an improvement and should go in, LGTM.

lib/Target/ARM/ARMSubtarget.cpp

364 ↗

(On Diff #113242)

I don't know if that is the original reason / motivation for it. But indeed the machine scheduler was not prepared/had bugs when dealing with instruction bundles last time I looked and so would probably fail on IT blocks.

[samparker]

Hi Matthias,

The reason for the inverse logic is because the decision isn't just based upon the CPU, but also the current mode. For the old CPUs, the post-RA scheduler should be used when compiling a function in arm mode, but not when in thumb mode because they only support Thumb1. I wanted to predicate on the Thumb2 capability, but then I would of still needed something else for cyclone and swift.

Thanks for the review,
sam

llvm/trunk/lib/Target/ARM/ARM.td

def FeatureNoNegativeImmediates
                                              "to their negated or complemented "
                                              "equivalent when the immediate does "
                                              "not fit in the encoding.">;
 
 // Use the MachineScheduler for instruction scheduling for the subtarget.
 def FeatureUseMISched: SubtargetFeature<"use-misched", "UseMISched", "true",
                                         "Use the MachineScheduler">;
 
-def FeaturePostRAScheduler : SubtargetFeature<"use-postra-scheduler",
-    "UsePostRAScheduler", "true", "Schedule again after register allocation">;
+def FeatureNoPostRASched : SubtargetFeature<"disable-postra-scheduler",
+    "DisablePostRAScheduler", "true",
+    "Don't schedule again after register allocation">;
 
 //===----------------------------------------------------------------------===//
 // ARM architecture class
 //
 
 // A-series ISA
 def FeatureAClass : SubtargetFeature<"aclass", "ARMProcClass", "AClass",
                                      "Is application profile ('A' series)">;
def : ProcessorModel<"swift",       SwiftModel,         [ARMv7a, ProcSwift,
                                                          FeatureHasSlowFPVMLx,
                                                          FeatureHasVMLxHazards,
                                                          FeatureProfUnpredicate,
                                                          FeaturePrefISHSTBarrier,
                                                          FeatureSlowOddRegister,
                                                          FeatureSlowLoadDSubreg,
                                                          FeatureSlowVGETLNi32,
                                                          FeatureSlowVDUP32,
-                                                         FeatureUseMISched]>;
+                                                         FeatureUseMISched,
+                                                         FeatureNoPostRASched]>;
 
 def : ProcessorModel<"cortex-r4",   CortexA8Model,      [ARMv7r, ProcR4,
                                                          FeatureHasRetAddrStack,
                                                          FeatureAvoidPartialCPSR]>;
 
 def : ProcessorModel<"cortex-r4f",  CortexA8Model,      [ARMv7r, ProcR4,
                                                          FeatureHasRetAddrStack,
                                                          FeatureSlowFPBrcc,
def : ProcessorModel<"cortex-r8",   CortexA8Model,      [ARMv7r,
                                                          FeatureMP,
                                                          FeatureSlowFPBrcc,
                                                          FeatureHWDivARM,
                                                          FeatureHasSlowFPVMLx,
                                                          FeatureAvoidPartialCPSR]>;
 
 def : ProcessorModel<"cortex-m3", CortexM3Model,        [ARMv7m,
                                                          ProcM3,
-                                                         FeatureHasNoBranchPredictor,
-                                                         FeaturePostRAScheduler]>;
+                                                         FeatureHasNoBranchPredictor]>;
 
 def : ProcessorModel<"sc300",     CortexM3Model,        [ARMv7m,
                                                          ProcM3,
                                                          FeatureHasNoBranchPredictor]>;
 
 def : ProcessorModel<"cortex-m4", CortexM3Model,        [ARMv7em,
                                                          FeatureVFP4,
                                                          FeatureVFPOnlySP,
                                                          FeatureD16,
-                                                         FeatureHasNoBranchPredictor,
-                                                         FeaturePostRAScheduler]>;
+                                                         FeatureHasNoBranchPredictor]>;
 
 def : ProcNoItin<"cortex-m7",                           [ARMv7em,
                                                          FeatureFPARMv8,
-                                                         FeatureD16,
-                                                         FeaturePostRAScheduler]>;
+                                                         FeatureD16]>;
 
 def : ProcNoItin<"cortex-m23",                          [ARMv8mBaseline,
                                                          FeatureNoMovt]>;
 
 def : ProcessorModel<"cortex-m33", CortexM3Model,       [ARMv8mMainline,
                                                          FeatureDSP,
                                                          FeatureFPARMv8,
                                                          FeatureD16,
                                                          FeatureVFPOnlySP,
-                                                         FeatureHasNoBranchPredictor,
-                                                         FeaturePostRAScheduler]>;
+                                                         FeatureHasNoBranchPredictor]>;
 
 def : ProcNoItin<"cortex-a32",                           [ARMv8a,
                                                          FeatureHWDivThumb,
                                                          FeatureHWDivARM,
                                                          FeatureCrypto,
                                                          FeatureCRC]>;
 
 def : ProcNoItin<"cortex-a35",                          [ARMv8a, ProcA35,
def : ProcessorModel<"cyclone",     SwiftModel,         [ARMv8a, ProcSwift,
                                                          FeatureMP,
                                                          FeatureHWDivThumb,
                                                          FeatureHWDivARM,
                                                          FeatureAvoidPartialCPSR,
                                                          FeatureAvoidMOVsShOp,
                                                          FeatureHasSlowFPVMLx,
                                                          FeatureCrypto,
                                                          FeatureUseMISched,
-                                                         FeatureZCZeroing]>;
+                                                         FeatureZCZeroing,
+                                                         FeatureNoPostRASched]>;
 
 def : ProcNoItin<"exynos-m1",                           [ARMv8a, ProcExynosM1,
                                                          FeatureHWDivThumb,
                                                          FeatureHWDivARM,
                                                          FeatureCrypto,
                                                          FeatureCRC]>;
 
 def : ProcNoItin<"exynos-m2",                           [ARMv8a, ProcExynosM1,

llvm/trunk/lib/Target/ARM/ARMScheduleR52.td

 // to subsequent operations before they are written to registers.
 // This scheduler is a MachineScheduler. See TargetSchedule.td for details.
 
 def CortexR52Model : SchedMachineModel {
   let MicroOpBufferSize = 0;  // R52 is in-order processor
   let IssueWidth = 2;         // 2 micro-ops dispatched per cycle
   let LoadLatency = 1;        // Optimistic, assuming no misses
   let MispredictPenalty = 8;  // A branch direction mispredict, including PFU
-  let PostRAScheduler = 1;    // Enable PostRA scheduler pass.
   let CompleteModel = 0;      // Covers instructions applicable to cortex-r52.
 }
 
 
 //===----------------------------------------------------------------------===//
 // Define each kind of processor resource and number available.
 
 // Modeling each pipeline as a ProcResource using the BufferSize = 0 since

llvm/trunk/lib/Target/ARM/ARMSubtarget.h

protected:
   bool InThumbMode = false;
 
   /// UseSoftFloat - True if we're using software floating point features.
   bool UseSoftFloat = false;
 
   /// UseMISched - True if MachineScheduler should be used for this subtarget.
   bool UseMISched = false;
 
-  /// UsePostRAScheduler - True if scheduling should happen again after
+  /// DisablePostRAScheduler - False if scheduling should happen again after
   /// register allocation.
-  bool UsePostRAScheduler = false;
+  bool DisablePostRAScheduler = false;
 
   /// HasThumb2 - True if Thumb2 instructions are supported.
   bool HasThumb2 = false;
 
   /// NoARM - True if subtarget does not support ARM mode execution.
   bool NoARM = false;
 
   /// ReserveR9 - True if R9 is not available as a general purpose register.
public:
   bool isAPCS_ABI() const;
   bool isAAPCS_ABI() const;
   bool isAAPCS16_ABI() const;
 
   bool isROPI() const;
   bool isRWPI() const;
 
   bool useMachineScheduler() const { return UseMISched; }
-  bool usePostRAScheduler() const { return UsePostRAScheduler; }
+  bool disablePostRAScheduler() const { return DisablePostRAScheduler; }
   bool useSoftFloat() const { return UseSoftFloat; }
   bool isThumb() const { return InThumbMode; }
   bool isThumb1Only() const { return InThumbMode && !HasThumb2; }
   bool isThumb2() const { return InThumbMode && HasThumb2; }
   bool hasThumb2() const { return HasThumb2; }
   bool isMClass() const { return ARMProcClass == MClass; }
   bool isRClass() const { return ARMProcClass == RClass; }
   bool isAClass() const { return ARMProcClass == AClass; }

llvm/trunk/lib/Target/ARM/ARMSubtarget.cpp

 bool ARMSubtarget::enableMachineScheduler() const {
   // Enable the MachineScheduler before register allocation for subtargets
   // with the use-misched feature.
   return useMachineScheduler();
 }
 
 // This overrides the PostRAScheduler bit in the SchedModel for any CPU.
 bool ARMSubtarget::enablePostRAScheduler() const {
-  if (usePostRAScheduler())
-    return true;
-  if (SchedModel.PostRAScheduler)
-    return true;
-  // No need for PostRA scheduling on subtargets where we use the
-  // MachineScheduler.
-  if (useMachineScheduler())
+  if (disablePostRAScheduler())
     return false;
-  return (!isThumb() || hasThumb2());
+  // Don't reschedule potential IT blocks.
+  return !isThumb1Only();
 }
 
 bool ARMSubtarget::enableAtomicExpand() const { return hasAnyDataBarrier(); }
 
 bool ARMSubtarget::useStride4VFPs(const MachineFunction &MF) const {
   // For general targets, the prologue can grow when VFPs are allocated with
   // stride 4 (more vpush instructions). But WatchOS uses a compact unwind
   // format which it's more important to get right.