[MISched] Add enableMachineScheduler function that checks enable-misched.

[fhahn]

Summary

Currently it seems like -enable-misched behaves differently to overriding
SubtargetInfo::enableMachineScheduler. The latter uses the source ordering in
SelectionDAGISel.cpp, while the former uses a more advanced scheduler.

This change updates SelectionDAGISel.cpp and MachineScheduler to use the same
criterion to decide whether to use the MachineScheduler or not. This patch is
work in progress, as quite a few test cases fail. So far I looked into the
ARM test failures and CodeGen/ARM/cortex-a57-misched-basic.ll fails because
the MachineScheduler falls back to source ordering and with -enable-misched the
instructions have been ordered optimally during SelectionDAGISel.

Full list of failing codegen tests:

Failing Tests (14):

LLVM :: CodeGen/AArch64/aarch64-a57-fp-load-balancing.ll
LLVM :: CodeGen/AArch64/arm64-aapcs.ll
LLVM :: CodeGen/AArch64/arm64-abi-varargs.ll
LLVM :: CodeGen/AArch64/arm64-abi.ll
LLVM :: CodeGen/AArch64/arm64-abi_align.ll
LLVM :: CodeGen/AArch64/arm64-misched-multimmo.ll
LLVM :: CodeGen/AArch64/arm64-vshift.ll
LLVM :: CodeGen/ARM/cortex-a57-misched-basic.ll
LLVM :: CodeGen/ARM/misched-fusion-aes.ll
LLVM :: CodeGen/X86/partial-fold32.ll
LLVM :: CodeGen/X86/partial-fold64.ll
LLVM :: CodeGen/X86/pr16031.ll
LLVM :: CodeGen/X86/win64_alloca_dynalloca.ll
LLVM :: CodeGen/X86/zext-fold.ll

[atrick]

I don't understand the motivation here. The purpose of command line flags is to test functionality in isolation. If you want a test case to control both SelectionDag and MISched functionality, then you use two command line flags.

The subtarget API controls the modes supported by the machine independent code. When a subtarget migrates to MachineScheduler it is supposed to migrate off SelectionDAG scheduling. Leaving both enabled is not something we ever intended to support and only hides problems with MachineScheduler.

[fhahn]

In D36381#834116, @atrick wrote:

I don't understand the motivation here. The purpose of command line flags is to test functionality in isolation. If you want a test case to control both SelectionDag and MISched functionality, then you use two command line flags.

My motivation is to make sure we test the MachineScheduler in isolation, when using -mi-sched. The current behavior hides problems with MachineScheduler on ARM, as -mi-sched enables the MachineScheduler, but keeps the original strategy for SelectionDAG.

I think when we enable the MachineScheduler via the command line, we should also use SelectionDAG with the source order strategy, which is what the patch does. AFAIK SelectionDAG needs to be run with the MachineScheduler to bring the selection graph back into a linear order, but it should use source order in that case and leave all scheduling decisions to MachineScheduler.

[atrick]

It sounds to me like you want a "MI-Sched Mode" convenience flag that does more than what it claims to do at face value, rather than needing to specify all the individual flags for SelectionDAG heuristic, enabling MI-Sched, coalescing heuristic. I'm not going to nit pick on the flag names because I don't have any stake it in. But I think you need to discuss this on llvm-dev and argue it out taking into consideration all of the goals:

• avoid confusions between the subtarget enableMachineScheduling() API and -mi-sched flag, which seems to have been a problem for you.

• provide "enable/disable" flags that don't do any magic beyond literally enabling or disabled the named pass, (that kind of magic been huge source of confusion in that past when it comes to triaging bugs and gathering performance data)

• unit tests should explicitly spell out precisely the passes/functionality that it depends on rather than relying on some umbrella "mode" that can change over time.

[fhahn]

In D36381#834280, @atrick wrote:

It sounds to me like you want a "MI-Sched Mode" convenience flag that does more than what it claims to do at face value, rather than needing to specify all the individual flags for SelectionDAG heuristic, enabling MI-Sched, coalescing heuristic. I'm not going to nit pick on the flag names because I don't have any stake it in. But I think you need to discuss this on llvm-dev and argue it out taking into consideration all of the goals:

• avoid confusions between the subtarget enableMachineScheduling() API and -mi-sched flag, which seems to have been a problem for you.

• provide "enable/disable" flags that don't do any magic beyond literally enabling or disabled the named pass, (that kind of magic been huge source of confusion in that past when it comes to triaging bugs and gathering performance data)

• unit tests should explicitly spell out precisely the passes/functionality that it depends on rather than relying on some umbrella "mode" that can change over time.

Agreed, I was just confused by the current behavior I think, but I don't think it's worth to change anything now.

include/llvm/CodeGen/MachineScheduler.h

 std::unique_ptr<ScheduleDAGMutation>
 createStoreClusterDAGMutation(const TargetInstrInfo *TII,
                               const TargetRegisterInfo *TRI);
 
 std::unique_ptr<ScheduleDAGMutation>
 createCopyConstrainDAGMutation(const TargetInstrInfo *TII,
                                const TargetRegisterInfo *TRI);
 
+bool enableMachineScheduler(const TargetSubtargetInfo &ST);
 } // end namespace llvm
 
 #endif // LLVM_CODEGEN_MACHINESCHEDULER_H

lib/CodeGen/MachineScheduler.cpp

 DefaultSchedRegistry("default", "Use the target's default scheduler choice.",
                      useDefaultMachineSched);
 
 static cl::opt<bool> EnableMachineSched(
     "enable-misched",
     cl::desc("Enable the machine instruction scheduling pass."), cl::init(true),
     cl::Hidden);
 
+namespace llvm {
+bool enableMachineScheduler(const TargetSubtargetInfo &ST) {
+  if (EnableMachineSched.getNumOccurrences()) {
+    if (!EnableMachineSched)
+      return false;
+  } else if (!ST.enableMachineScheduler())
+    return false;
+  return true;
+}
+} // end namespace llvm
+
 static cl::opt<bool> EnablePostRAMachineSched(
     "enable-post-misched",
     cl::desc("Enable the post-ra machine instruction scheduling pass."),
     cl::init(true), cl::Hidden);
 
 /// Decrement this iterator until reaching the top or a non-debug instr.
 static MachineBasicBlock::const_iterator
 priorNonDebug(MachineBasicBlock::const_iterator I,

 /// scheduler must update the RegionBegin and RegionEnd positions cached by
 /// ScheduleDAGInstrs whenever adding or removing instructions. A much simpler
 /// design would be to split blocks at scheduling boundaries, but LLVM has a
 /// general bias against block splitting purely for implementation simplicity.
 bool MachineScheduler::runOnMachineFunction(MachineFunction &mf) {
   if (skipFunction(*mf.getFunction()))
     return false;
 
-  if (EnableMachineSched.getNumOccurrences()) {
-    if (!EnableMachineSched)
-      return false;
-  } else if (!mf.getSubtarget().enableMachineScheduler())
+  if (!enableMachineScheduler(mf.getSubtarget()))
     return false;
 
   DEBUG(dbgs() << "Before MISched:\n"; mf.print(dbgs()));
 
   // Initialize the context of the pass.
   MF = &mf;
   MLI = &getAnalysis<MachineLoopInfo>();
   MDT = &getAnalysis<MachineDominatorTree>();

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineMemOperand.h"
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/MachinePassRegistry.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
+#include "llvm/CodeGen/MachineScheduler.h"
 #include "llvm/CodeGen/MachineValueType.h"
 #include "llvm/CodeGen/SchedulerRegistry.h"
 #include "llvm/CodeGen/SelectionDAG.h"
 #include "llvm/CodeGen/SelectionDAGISel.h"
 #include "llvm/CodeGen/SelectionDAGNodes.h"
 #include "llvm/CodeGen/StackProtector.h"
 #include "llvm/CodeGen/ValueTypes.h"
 #include "llvm/IR/BasicBlock.h"
ScheduleDAGSDNodes* createDefaultScheduler(SelectionDAGISel *IS,
     const TargetSubtargetInfo &ST = IS->MF->getSubtarget();
 
     // Try first to see if the Target has its own way of selecting a scheduler
     if (auto *SchedulerCtor = ST.getDAGScheduler(OptLevel)) {
       return SchedulerCtor(IS, OptLevel);
     }
 
     if (OptLevel == CodeGenOpt::None ||
-        (ST.enableMachineScheduler() && ST.enableMachineSchedDefaultSched()) ||
+        (enableMachineScheduler(ST) && ST.enableMachineSchedDefaultSched()) ||
         TLI->getSchedulingPreference() == Sched::Source)
       return createSourceListDAGScheduler(IS, OptLevel);
     if (TLI->getSchedulingPreference() == Sched::RegPressure)
       return createBURRListDAGScheduler(IS, OptLevel);
     if (TLI->getSchedulingPreference() == Sched::Hybrid)
       return createHybridListDAGScheduler(IS, OptLevel);
     if (TLI->getSchedulingPreference() == Sched::VLIW)
       return createVLIWDAGScheduler(IS, OptLevel);