Page MenuHomePhabricator

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

Authored by fhahn on Aug 6 2017, 10:52 AM.

Details

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

Diff Detail

Event Timeline

fhahn created this revision.Aug 6 2017, 10:52 AM
atrick edited edge metadata.Aug 7 2017, 10:47 AM

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 added a comment.Aug 7 2017, 12:23 PM

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 added a comment.EditedAug 7 2017, 12:39 PM

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 abandoned this revision.Aug 17 2017, 3:35 AM

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.