[PPC] Allow two post RA schedulers to be in the pipeline and select one depending on the Machine Function's subtarget

[echristo]

Summary

For PWR9, PPC backend migrates to the new Post RA MI scheduler. We continue to use PostRA top down list scheduler for PWR8 and older processors. Given that each function can override module's target-cpu, we need a mechanism that allows us to use proper scheduler, depending on the function's target-cpu.

The solution here has the following components:

1- We add a new method MachineFunctionPass::skipFunction (const MachineFunction &) which internally passes the pass ID to Machine Function's subtarget and ask whether this pass should run for this subtarget or not. If Subtarget is OK, we then internally call FunctionPass::skipFunction.

2- A new virtual getPassID method is added to MachineFunctionPass. Each subclass, can override this to return proper ID. This is used in the skipFunction of (1).

3- Some refactoring in TargetPassConfig and PPCTargetMachine is done to allow PPC to add both PostRA schedulers to the pipeline and disable the post RA scheduler with command line options.

4- TargetSubtargetInfo now has a virtual runPass method that is called by the new skipFunction method of (1).

I have tested this by checking which pass is run when invoking llc and pass different value for mcpu option. To commit those tests, I would need to run test that rely on checking DEBUG output of these passes. I am not clear if committing this kind of testcase is OK/necessary or not. Comments are welcome. I have checked that -print-after= option works properly with these changes. I have not checked if the subtarget constructed for a function that overrides target-cpu contains all necessary information. But I believe that is a separate problem. We have to check that at some point and fix any bug that we find. (I suspect that the project that allowed functions to override target-cpus has done that check....)

[MatzeB]

This adds a bunch of infrastructure in a bad way! This should be handled with a Subtarget callback like TargetSubtargetInfo::enablePostRAScheduler() which already exists!

include/llvm/CodeGen/MachineFunctionPass.h

34 ↗

(On Diff #74274)

What is this good for? It seems unrelevant to the patch and this is just a base class nothing that can/should be scheduled or end up in the pass registry.

[MatzeB]

I think I should be more specific in my critique and I noticed that I missed part of the problem while reading this the first time:

• I'm against adding infarstructure/special cases at a basic level like MachineFunctionPass. It is already more complicated than it should be today.
• The current way of replacing the PostRAScheduler with the MachinePostRAScheduler is messy indeed. How about always scheduling both passes and letting enablePostRAScheduler() return an enum value indicating whether we want the machine or the list scheduler to run?

[amehsan]

In D25482#567561, @MatzeB wrote:

I think I should be more specific in my critique and I noticed that I missed part of the problem while reading this the first time:

• I'm against adding infarstructure/special cases at a basic level like MachineFunctionPass. It is already more complicated than it should be today.
• The current way of replacing the PostRAScheduler with the MachinePostRAScheduler is messy indeed. How about always scheduling both passes and letting enablePostRAScheduler() return an enum value indicating whether we want the machine or the list scheduler to run?

Thanks. First of all part of the solution that you are proposing is what is being done in the patch. Both passes are being added to the pipeline. You are suggesting a somewhat different approach for each pass to decide whether it should run or not. I think using enablePostRAScheduler() pollutes subtarget classes. Details follow.

The problem is that, if I want to take this approach then:

1- Subtarget has a method devoted to turning on or off PostRA scheduling.
2- Subtarget has to be aware of different PostRA scheduling passes that we have (to be able to pass proper enum values).

The solution in this patch provides one general solution for subtargets to answer queries about whether a pass should run or not. It also relies on existing Pass IDs. If this problem pops up again for a different pass or a different platform, all we need to do is to change the platform's runPass method that I have introduced here (and add a couple of lines to it). Also the given pass will need to change the skipFunction method that it calls. (all that is needed for that, is to remove a few characters of the code, as you can see in this patch).

Also please note that the method enablePostRASchedule can be removed from the subtarget classes with the infrastructure introduced in this patch. I can open a PR so we can do that cleanup later.

In comparison, the solution proposed in this comment, requires having a method corresponding to the given pass in the subtarget AND also making subtarget aware of PassIDs or something equivalent (like enums that you are proposing).

Just to give you an example that this is not an isolated problem, I can mention modulo scheduling pass. If we want to turn that on for PPC most likely it will be only for PWR9 not earlier processors. So do we need to add a new method enableMachinePipelining to the subtarget? I think it will be cleaner to use the infrastructure introduced in this patch. Call proper overload of skipFunction and add a couple of lines to runPass method and check for the ID of that pass.

If we come to the conclusion that this is the right approach for solving this problem, the fact that MachineFunctionPass is already complicated is beside the point. If something should not be in there, it should be removed. If we need to refactor it to handle its complexity that is a different problem to tackle.

[MatzeB]

In D25482#567576, @amehsan wrote:

In D25482#567561, @MatzeB wrote:

I think I should be more specific in my critique and I noticed that I missed part of the problem while reading this the first time:

• I'm against adding infarstructure/special cases at a basic level like MachineFunctionPass. It is already more complicated than it should be today.
• The current way of replacing the PostRAScheduler with the MachinePostRAScheduler is messy indeed. How about always scheduling both passes and letting enablePostRAScheduler() return an enum value indicating whether we want the machine or the list scheduler to run?

Thanks. First of all part of the solution that you are proposing is what is being done in the patch. Both passes are being added to the pipeline.

Well I was proposing to always schedule both passes in TargetPassConfig rather than requiring interested targets to overload a newly introduced addPostRASched() functions to keep things simpler.

You are suggesting a somewhat different approach for each pass to decide whether it should run or not. I think using enablePostRAScheduler() pollutes subtarget classes. Details follow.

The problem is that, if I want to take this approach then:

1- Subtarget has a method devoted to turning on or off PostRA scheduling.
2- Subtarget has to be aware of different PostRA scheduling passes that we have (to be able to pass proper enum values).

I think the fact that you propose this very patch shows that the choice of post-ra scheduling algorithm is subtarget specific hence subtargets have to potentially be aware of it. In any way implementers can choose not to override that function and go with the default (which enables the machine scheduler or nothing depending on the choice made in the tablegen/MCSchedule).

The solution in this patch provides one general solution for subtargets to answer queries about whether a pass should run or not. It also relies on existing Pass IDs. If this problem pops up again for a different pass or a different platform, all we need to do is to change the platform's runPass method that I have introduced here (and add a couple of lines to it). Also the given pass will need to change the skipFunction method that it calls. (all that is needed for that, is to remove a few characters of the code, as you can see in this patch).

I am being wary about the "general solution" part. The pattern adds an extra non-obvious possibility why a pass wouldn't run, which has its costs when reading and debugging any pass. There is exactly one runPass() function which has to handle a combination of subtarget + passid to make a decision, this is very likely to get unwieldy as more passes and more subtarget exceptions get added in the future. Having 1 callback per pass (like enablePostRAScheduling) at least keeps the passid dimension out of it and makes it easier to search and understand the behaviour IMO.

Also please note that the method enablePostRASchedule can be removed from the subtarget classes with the infrastructure introduced in this patch. I can open a PR so we can do that cleanup later.

In comparison, the solution proposed in this comment, requires having a method corresponding to the given pass in the subtarget AND also making subtarget aware of PassIDs or something equivalent (like enums that you are proposing).

Just to give you an example that this is not an isolated problem, I can mention modulo scheduling pass. If we want to turn that on for PPC most likely it will be only for PWR9 not earlier processors. So do we need to add a new method enableMachinePipelining to the subtarget? I think it will be cleaner to use the infrastructure introduced in this patch. Call proper overload of skipFunction and add a couple of lines to runPass method and check for the ID of that pass.

Given the current choices I rather see enableMachinePipelining() added than a generic skipPass() callback.

If we come to the conclusion that this is the right approach for solving this problem, the fact that MachineFunctionPass is already complicated is beside the point. If something should not be in there, it should be removed. If we need to refactor it to handle its complexity that is a different problem to tackle.

Well I believe there are reasonable specific solutions here so we do not need to increase the general complexity.

[amehsan]

The pattern adds an extra non-obvious possibility why a pass wouldn't run, which has its costs when reading and debugging any pass.

Not sure I follow here. Note that we have two overload of skipFunction(). Any pass that does not want to call the subtarget, can call the current overload. Any pass that calls the new overload, will need to call subtarget one way or another. So the extra check is there in both cases.

There is exactly one runPass() function which has to handle a combination of subtarget + passid to make a decision, this is very likely to get unwieldy as more passes and more subtarget exceptions get added in the future.

In most cases this function looks at (cpu, pass) pair and return true/false based on that. Any subtarget that starts to have a complicated runPass, can engineer their code properly. For example by adding private functions for each pass, and calling the private function based on pass id. (Note that the difference with having enablePostRA... function is that the pass dependent extra function is no longer part of the subtarget interface).

Having 1 callback per pass (like enablePostRAScheduling) at least keeps the passid dimension out of it and makes it easier to search and understand the behaviour IMO.

I see two problems with this. The minor problem is this: for the example that we have discussed so far, we need one enablePostRA function that returns an enum and an enableMachinePipeliner that returns a bool. This is a minor advantage for the current patch in terms of code quality. (In addition to a less polluted interface for subtarget). The more important problem is related to the following:

Well I was proposing to always schedule both passes in TargetPassConfig rather than requiring interested targets to overload a newly introduced addPostRASched() functions to keep things simpler.

This is not a good idea. There are already backends that have stopped using Post RA top down scheduler and use the new MI scheduler. Also the general idea, AFAIK, is that the top down scheduler is the old mechanism and MI scheduler is the new mechanism. With what you are proposing, we are saying that people who have moved/will move to the new scheduler (or potentially any new subtarget that starts using new mechanism from the beginning), should continue calling the old scheduler until every backend has moved off of it so we can entirely remove it. I don't think this is a good idea. This reminds me of the first point of your comment that I quoted here: "The pattern adds an extra non-obvious possibility why a pass wouldn't run, which has its costs when reading and debugging any pass.". Not exactly the same issue, but similar.

[MatzeB]

In D25482#569399, @amehsan wrote:

The pattern adds an extra non-obvious possibility why a pass wouldn't run, which has its costs when reading and debugging any pass.

Not sure I follow here. Note that we have two overload of skipFunction(). Any pass that does not want to call the subtarget, can call the current overload. Any pass that calls the new overload, will need to call subtarget one way or another. So the extra check is there in both cases.

There is exactly one runPass() function which has to handle a combination of subtarget + passid to make a decision, this is very likely to get unwieldy as more passes and more subtarget exceptions get added in the future.

In most cases this function looks at (cpu, pass) pair and return true/false based on that. Any subtarget that starts to have a complicated runPass, can engineer their code properly. For example by adding private functions for each pass, and calling the private function based on pass id. (Note that the difference with having enablePostRA... function is that the pass dependent extra function is no longer part of the subtarget interface).

Having 1 callback per pass (like enablePostRAScheduling) at least keeps the passid dimension out of it and makes it easier to search and understand the behaviour IMO.

I see two problems with this. The minor problem is this: for the example that we have discussed so far, we need one enablePostRA function that returns an enum and an enableMachinePipeliner that returns a bool. This is a minor advantage for the current patch in terms of code quality. (In addition to a less polluted interface for subtarget). The more important problem is related to the following:

I don't see the code becoming cleaner here. Instead of a targeted patch that just affects the behavior of PostMachineScheduler and PostRASchedulerList you add a generic mechanism that affects every pass calling skipFunction(). I don't see the majority of passes using this functionality.
So what you see as helpful/useful utility for other passes, mainly strikes me as a (small but not neglibile) increase in accidental complexity.

Well I was proposing to always schedule both passes in TargetPassConfig rather than requiring interested targets to overload a newly introduced addPostRASched() functions to keep things simpler.

This is not a good idea. There are already backends that have stopped using Post RA top down scheduler and use the new MI scheduler. Also the general idea, AFAIK, is that the top down scheduler is the old mechanism and MI scheduler is the new mechanism. With what you are proposing, we are saying that people who have moved/will move to the new scheduler (or potentially any new subtarget that starts using new mechanism from the beginning), should continue calling the old scheduler until every backend has moved off of it so we can entirely remove it. I don't think this is a good idea. This reminds me of the first point of your comment that I quoted here: "The pattern adds an extra non-obvious possibility why a pass wouldn't run, which has its costs when reading and debugging any pass.". Not exactly the same issue, but similar.

I don't see the point. This is one more pass scheduled that most likely immediately boils out at the beginning of runOnMachineFunction(), I don't see how that is worse than adding one more if() to every MachinePass. Sure I would love to not have to schedule two passes, so go ahead and upgrade your subtargets PPC! Until then I am fine with the simple solution.

[amehsan]

you add a generic mechanism that affects every pass calling skipFunction().

This patch has zero impact on any pass other than the two PostRA schedulers.

I don't see how that is worse than adding one more if() to every MachinePass.

Why do you think this patch adds one more if() to every MachinePass? The impact on any pass other than the two PostRA schedulers, is literally zero.

[MatzeB]

In D25482#571024, @amehsan wrote:

you add a generic mechanism that affects every pass calling skipFunction().

This patch has zero impact on any pass other than the two PostRA schedulers.

I don't see how that is worse than adding one more if() to every MachinePass.

Why do you think this patch adds one more if() to every MachinePass? The impact on any pass other than the two PostRA schedulers, is literally zero.

Oh you overload skipFunction, it really looks like an override given that there is already a skipFunction in the base class, that is confusing. And I still don't think the patch should add things to MachineFunctionPass.

include/llvm/CodeGen/MachineFunctionPass.h

74–75 ↗

(On Diff #74274)

This is highly confusing to have different behavior based on which overload you choose, there is not even a comment here describing the difference.

include/llvm/Target/TargetSubtargetInfo.h

225–228 ↗

(On Diff #74274)

This does not mention that this will only work if the MachineFunction variant of skipFunction is used.

lib/Target/PowerPC/PPCSubtarget.cpp

263 ↗

(On Diff #74274)

This cannot be reached and can be removed.

lib/Target/PowerPC/PPCTargetMachine.cpp

439–440

indentation

444–450

I don't see what is PPC specific about this, maybe put it into TargetPassConfig? It may not be necessary though as there is already -enable-post-misched=false.

445

indentation, please check clang-format.

[MatzeB]

include/llvm/CodeGen/MachineFunctionPass.h

34 ↗

(On Diff #74274)

We should not need to define an ID for the base class as we never put a bare MachineFunctionPass into the PassManager pipeline.

70–72 ↗

(On Diff #74274)

There is already AnalysisID getPassID() const in the base class which does that (just returns a AnalysisID pointer instead of a reference).

[amehsan]

In D25482#571885, @MatzeB wrote:

In D25482#571024, @amehsan wrote:

you add a generic mechanism that affects every pass calling skipFunction().

This patch has zero impact on any pass other than the two PostRA schedulers.

I don't see how that is worse than adding one more if() to every MachinePass.

Why do you think this patch adds one more if() to every MachinePass? The impact on any pass other than the two PostRA schedulers, is literally zero.

Oh you overload skipFunction, it really looks like an override given that there is already a skipFunction in the base class, that is confusing. And I still don't think the patch should add things to MachineFunctionPass.

:) OK. I had mentioned a couple of times in my earlier comments that this is an overload, but I guess long comments are mostly glanced over. So I will remove the getPassID function and the static ID field that I added and instead use existing getPassID. (I did not realize that this exists. Thanks for pointing it out :). I will also add comments before declaration of each skipFunction overload (and other places that you mentioned) to clarify that there exists another overload.

include/llvm/CodeGen/MachineFunctionPass.h

74–75 ↗

(On Diff #74274)

I will add comment. If I understand correctly the using clause would not be needed if this was an override. That should indicate that this is an overload. But a comment will definitely help.

lib/Target/PowerPC/PPCSubtarget.cpp

263 ↗

(On Diff #74274)

OK

lib/Target/PowerPC/PPCTargetMachine.cpp

439–440

OK

444–450

I need to make sure that if someone compile with -mcpu=pwr9 and there are functions with target-cpu=pwr8, still with -disable-post-ra option passed to the command line all post ra schedulers will be turned off.

445

OK

[MatzeB]

I just happened to run accross this patch: https://reviews.llvm.org/D8717

So there is precedent on how to achieve this, we should probably use this style for consistency.

[amehsan]

In D25482#578083, @MatzeB wrote:

I just happened to run accross this patch: https://reviews.llvm.org/D8717

So there is precedent on how to achieve this, we should probably use this style for consistency.

Thanks. Will look into it.

[amehsan]

In D25482#578165, @amehsan wrote:

In D25482#578083, @MatzeB wrote:

I just happened to run accross this patch: https://reviews.llvm.org/D8717

So there is precedent on how to achieve this, we should probably use this style for consistency.

Thanks. Will look into it.

My main concern with the approach of D8717 is that now I will need to change functions like createGenericSchedPostRA() and either create Target Specific overrides for that or add some target specific code to it (get PPCSubtarget, if not null then...). I think both of these are ugly and more invasive.

[amehsan]

In D25482#578581, @amehsan wrote:

In D25482#578165, @amehsan wrote:

In D25482#578083, @MatzeB wrote:

I just happened to run accross this patch: https://reviews.llvm.org/D8717

So there is precedent on how to achieve this, we should probably use this style for consistency.

Thanks. Will look into it.

My main concern with the approach of D8717 is that now I will need to change functions like createGenericSchedPostRA() and either create Target Specific overrides for that or add some target specific code to it (get PPCSubtarget, if not null then...). I think both of these are ugly and more invasive.

As for consistency, I think the approach here can replace both D8717 and enablePostRAScheduler() approaches. We need a PR to remove those in favor of the simpler and more general approach.

[amehsan]

Even though we may continue discussing the approach, I update the patch to

1- Use existing getPassID instead of introducing a new one.
2- Adding more comments to the code.

[MatzeB]

Will this patch become obsolete when darwin support is removed from the PowerPC target (see http://lists.llvm.org/pipermail/llvm-dev/2016-October/106359.html)? Even if we would really decide to keep PPC support, I am pretty sure that in the interest of easy maintainability we should not have an alternate scheduling strategy just for darwin...

[amehsan]

In D25482#578886, @MatzeB wrote:

Will this patch become obsolete when darwin support is removed from the PowerPC target (see http://lists.llvm.org/pipermail/llvm-dev/2016-October/106359.html)? Even if we would really decide to keep PPC support, I am pretty sure that in the interest of easy maintainability we should not have an alternate scheduling strategy just for darwin...

No, this patch has nothing to do with Darwin support. I know that I am checking DarwinDirective somewhere. That is because I saw that pattern used to check which cpu we are running on elsewhere in the code. I can change that line of code (if that directive becomes obsolete after we drop Darwin support), but otherwise this patch is mostly about PWR8 and PWR9 and has nothing to do with darwin support.

[MatzeB]

In D25482#578907, @amehsan wrote:

In D25482#578886, @MatzeB wrote:

Will this patch become obsolete when darwin support is removed from the PowerPC target (see http://lists.llvm.org/pipermail/llvm-dev/2016-October/106359.html)? Even if we would really decide to keep PPC support, I am pretty sure that in the interest of easy maintainability we should not have an alternate scheduling strategy just for darwin...

No, this patch has nothing to do with Darwin support. I know that I am checking DarwinDirective somewhere. That is because I saw that pattern used to check which cpu we are running on elsewhere in the code. I can change that line of code, but otherwise this patch is mostly about PWR8 and PWR9 and has nothing to do with darwin support.

Ok, checking DarwinDirective to differentiate between PWR8 and PWR9 is confusing for the uninitiated :)

[amehsan]

In D25482#578923, @MatzeB wrote:

In D25482#578907, @amehsan wrote:

In D25482#578886, @MatzeB wrote:

Will this patch become obsolete when darwin support is removed from the PowerPC target (see http://lists.llvm.org/pipermail/llvm-dev/2016-October/106359.html)? Even if we would really decide to keep PPC support, I am pretty sure that in the interest of easy maintainability we should not have an alternate scheduling strategy just for darwin...

No, this patch has nothing to do with Darwin support. I know that I am checking DarwinDirective somewhere. That is because I saw that pattern used to check which cpu we are running on elsewhere in the code. I can change that line of code, but otherwise this patch is mostly about PWR8 and PWR9 and has nothing to do with darwin support.

Ok, checking DarwinDirective to differentiate between PWR8 and PWR9 is confusing for the uninitiated :)

You are right :) Unfortunately that seems to be fairly common in the code, and the alternative that I found was checking CPU string. I thought this is still better than checking a string. Definitely we need to improve that.

[hfinkel]

In D25482#578928, @amehsan wrote:

In D25482#578923, @MatzeB wrote:

In D25482#578907, @amehsan wrote:

In D25482#578886, @MatzeB wrote:

Will this patch become obsolete when darwin support is removed from the PowerPC target (see http://lists.llvm.org/pipermail/llvm-dev/2016-October/106359.html)? Even if we would really decide to keep PPC support, I am pretty sure that in the interest of easy maintainability we should not have an alternate scheduling strategy just for darwin...

No, this patch has nothing to do with Darwin support. I know that I am checking DarwinDirective somewhere. That is because I saw that pattern used to check which cpu we are running on elsewhere in the code. I can change that line of code, but otherwise this patch is mostly about PWR8 and PWR9 and has nothing to do with darwin support.

Ok, checking DarwinDirective to differentiate between PWR8 and PWR9 is confusing for the uninitiated :)

You are right :) Unfortunately that seems to be fairly common in the code, and the alternative that I found was checking CPU string. I thought this is still better than checking a string. Definitely we need to improve that.

Yea, we've never renamed the variable. We should.

[amehsan]

Yea, we've never renamed the variable. We should.

Opened a PR for this

[kbarton]

I'm OK with this, as long as all of @MatzeB issues are addressed.
It would be good to get comments from either @hfinkel or @echristo as well.

[amehsan]

@MatzeB
Do you have further comments on this?

[echristo]

lib/CodeGen/MachineFunctionPass.cpp

94–98 ↗

(On Diff #75728)

I'm not sure why you need all of this and can't write the skipFunction without needing a MachineFunction in specific. Can you explain for me?

[amehsan]

lib/CodeGen/MachineFunctionPass.cpp

94–98 ↗

(On Diff #75728)

Because I need MachineFunction's subtarget. Does that answer your question?

[echristo]

lib/CodeGen/MachineFunctionPass.cpp

94–98 ↗

(On Diff #75728)

Not really, but I'd missed any resolution (which didn't appear to happen) for the basic idea of passing in a function to the pass.

Can this be done similar to IfConversion instead? I'd really prefer not to have this overloading of the idea of skipFunction.

[amehsan]

lib/CodeGen/MachineFunctionPass.cpp

94–98 ↗

(On Diff #75728)

I don't really understand what you mean by "the basic idea of passing in a function to the pass". Could you clarify?

By IfConversion I believe you are referring to D8717. Could you explain why you prefer that to overloading skipFunction?

[echristo]

lib/CodeGen/MachineFunctionPass.cpp

94–98 ↗

(On Diff #75728)

So D8717 allows targets to configure passes by passing in a callback at configuration time. skipFunction is a pretty terrible hack that originally existed as a mechanism to deal with optnone and has an ok side effect of being able to use it for bisection. It's not a use case that should be promulgated further, especially given that the configuration via call back is both easier to understand and just as configurable (and more localized).

[amehsan]

lib/CodeGen/MachineFunctionPass.cpp

94–98 ↗

(On Diff #75728)

Let me try to explain why "skipFunction is a pretty terrible hack"

Ideally, when we have a noopt function, no optimization pass should be invoked. Our infrastructure does not allow us to do that, so we use this hack that each optimization first checks the function if it is noopt or not.

Is this correct? If yes, are there other reason as well or not. If no, why skipFunction is a hack?

[amehsan]

lib/CodeGen/MachineFunctionPass.cpp

94–98 ↗

(On Diff #75728)

I wanted to edit my comment if possible, and clicked on "hide comment". Let me repeat it.

Let me try to explain why "skipFunction is a pretty terrible hack"

Ideally, when we have a noopt function, no optimization pass should be invoked. For some reason we cannot do that, so we use this hack that each optimization first checks the function if it is noopt or not.

Is this correct? If yes, are there other reason as well? If no, why skipFunction is a hack?

[echristo]

lib/CodeGen/MachineFunctionPass.cpp

94–98 ↗

(On Diff #75728)

It's a bit more complicated than that, it also involves how the code generator is constructed, the problems with the DAG combiner, various code generators not handling code that the DAG combiner hasn't run on, etc. You should take a look at the threads around the time the code came in about it. I believe Paul Robinson and I were part of the discussions here.

There's also my dislike of hardcoding pass IDs in the backend - I think that's poor maintenance and more confusing than a simple function passed in at pass construction time. Do you disagree? If so, can you cite an example of why?

Let me rephrase your question back at you: Is there a good reason for not doing it the way I'm proposing and have used across all of the backends to enable/disable passes based on subtargets?

[amehsan]

lib/CodeGen/MachineFunctionPass.cpp

94–98 ↗

(On Diff #75728)

Before I answer your question, let me say that I can completely decouple this patch from skipFunction by a few small changes.That should address your concern of promulgating skipFunction.

With D8717, there is extra boilerplate code for creating, passing and checking the subtarget callback to the pass. This boilerplate code should be repeated for each pass that is subtarget dependent. We need all of this, despite the fact that a subtarget object is already available through MachineFunction object. My proposed patch, minimizes the boilerplate code and uses the available subtarget object. It is simpler and it is more general.

Additionally, from a quick look, I am not sure that implementing D8717 approach for PostRA MIScheduler is as straightforward as IfConversion, given creation of scheduler seems to be more complicated. Again, this is my impression from a quick look at the code.

I have found an email thread from early 2014 which should be what you mentioned. I have not yet gone through it to see if I find a reason there against this idea or not. If there is a reason, that explains why the extra boilerplate of 8717 is required, I can happily look into what it takes to apply that idea to PostRA scheduler.

[echristo]

lib/CodeGen/MachineFunctionPass.cpp

94–98 ↗

(On Diff #75728)

I feel there's very little boilerplate added for a single pass. I would definitely prefer that mechanism. While the MI Scheduler code is more complicated than IfConversion I'd prefer you work on integrating this style of callback and simplifying the code that's currently there (i.e. removing the current way the command line option works if necessary) than the current direction you're going.

[amehsan]

lib/CodeGen/MachineFunctionPass.cpp

94–98 ↗

(On Diff #75728)

OK. I will update the patch.

[amehsan]

lib/Target/PowerPC/PPCTargetMachine.cpp

448

I need to make sure that the lower bound here (PPC::DIR_32) is correct.

[MatzeB]

This looks pretty good. I just have some small nitpicks left:

include/llvm/CodeGen/Passes.h

120–121

I recently changed the callback for IfConversion/ExpandBundle to take a MachineFunction& instead of a Function& which should also make life slightly easier in this case.

181–182

Same as above, use MachineFunction&

include/llvm/CodeGen/TargetPassConfig.h

256–257

That fact that targets can override these functions is true for the whole file. I'd just state what the function is supposed to be doing. Something like
Add scheduler passes to be executed after register allocation.

258

Rename this to addPostRASched, a target could just as well add multiple passes or none at all.

lib/CodeGen/MachineScheduler.cpp

368–369

There is an interesting difference to IfConversion here which first checks skipFunction and then performs this test. Thinking about it I think this variant is preferable in the pass bisection use case. So we can keep this and should change IfConversion in a subsequent patch.

lib/CodeGen/TargetPassConfig.cpp

273–276

This comment should go to TargetPassConfig.h.

284–295

This comment should go to TargetPassConfig.h.

537–539

This comment seems redundant with the doxygen comment for the whole addPostRASchedPass() so we can remove it.

lib/Target/PowerPC/PPCTargetMachine.cpp

441

This can be simplified to MF.getSubtarget<PPCSubtarget>().getDarwinDirective() when the argument is changed to MachineFunction& (see above). Similar in the next callback.

[MatzeB]

lib/CodeGen/TargetPassConfig.cpp

541–542

It looks like we supersede ™::targetSchedulesPostRAScheduling()"? It should be trivially for the targets reporting true here to instead just overload this method with an empty one. (We can do this in a follow up patch though).

[amehsan]

lib/CodeGen/TargetPassConfig.cpp

541–542

My understanding is that if a target returns true, it means that they probably want to schedule PostRAScheduler pass somewhere else in the pipeline. I think this condition should guard the call to addPostRASchedPass(), even though the functionality will be the same.

[amehsan]

lib/CodeGen/TargetPassConfig.cpp

541–542

I also leave this comment here as it talks about the functionality of targetSchedulesPostRAScheduling. I think we need a PR to clean up all the different ways and method of scheduling post ra sched and remove redundants and those that don't have a use case. There is the virtual function and this targetSchedulesPostRAScheduling() and enablePostRA.... and various methods of choosing between old scheduler and new scheduer (command line, substitutePass)

[MatzeB]

LGTM, thanks for working on this.
I couldn't stop myself marking a few more nitpicks, but there is no further review necessary.

lib/CodeGen/MachineScheduler.cpp

140

We usually don't have an empty line at the beginning of a class.

367

We usually do no have a newline at the beginning of a function. Same for PostRAScheduler::runOnMachineFunction.

lib/CodeGen/PostRASchedulerList.cpp

717

No newline at end of file.

lib/CodeGen/TargetPassConfig.cpp

541–542

Yes, this was meant as a side remark and as I said we can deal with it another time, not as part of this patch.

lib/Target/PowerPC/PPCTargetMachine.cpp

306

We usually do not have a newline before the end of a class.

[amehsan]

Eric, asked for a little more clean up.

I am aware of the following mechanisms for scheduling post ra schedulers.

1- The new virtual function in this patch
2- targetSchedulesPostRAScheduling
3- enablePostRA method in the subtarget
4- PostRAScheduler flag in include/llvm/Target/TargetSchedule.td
5- command line option to turn on new scheduler instead of old one.
6- command line option to turn off post ra schedulers
7- SubstitutePass to use new scheduler instead of old one.

Each of these seems to have a slightly different usecase. If there is a likelihood that we can remove some of these, it might be worthwhile to open a PR. otherwise we can leave it as is.

Matt: Sorry I just realized I could have applied your comment to this update. I will make sure to check those before committing though.

[echristo]

lib/CodeGen/TargetPassConfig.cpp

541–542

When were you planning on tackling this? I think unraveling this as part of a prepartory patch and splitting out the necessary elements would be general project goodness.

[MatzeB]

Indeed there are number of mechanisms in place nowadays, so for what it's worth:

In D25482#584970, @amehsan wrote:

Eric, asked for a little more clean up.

I am aware of the following mechanisms for scheduling post ra schedulers.

1- The new virtual function in this patch

I consider overriding methods in TargetPassConfig to be the most canonical way for targets to influence pass scheduling. So this should stay.

2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

3- enablePostRA method in the subtarget
4- PostRAScheduler flag in include/llvm/Target/TargetSchedule.td

This redundancy is unfortunate, IMO we should just have the one in TargetSchedModel/TargetSchedule.td and not an extra callback in TargetSubtargetInfo. Last time I tried fixing this I ran into the problem that aarch64 reuses a scheduling model for two targets with different postrasched settings. One way to attack this would be to figure out if we may not better share the same postra setting or whether we should duplicate the scheduling model with different postra sched settings. The other thing which I would like to see longer term is having a way to subclass TargetSchedModel so targets can specialize a few more scheduling related functions (there is some functions like shouldScheduleAdjacent() or isAsCheapAsMove() which I think would make more sense as part of a per-subtarget scheduling model). I'd really like to see some cleanups here but don't think we should block this patch (at least not on this specific point).

5- command line option to turn on new scheduler instead of old one.
6- command line option to turn off post ra schedulers

I don't think we need to spend too much time thinking about our debugging commandline options. And those two are fine IMO.

7- SubstitutePass to use new scheduler instead of old one.

I never liked this mechanism. I believe it should be possible to (re-)model TargetPassConfig in a way that there is a method that can be overridden to replace a pass. (I also believe that we could simplify target passmanager configuration by doing the nonintuitive move of actually duplicating some of the setup code across our targets so they have an easier time creating variants/deviating from the default, but I can see this becoming a controversial debate I'd like to not start in this review ;-) )

Each of these seems to have a slightly different usecase. If there is a likelihood that we can remove some of these, it might be worthwhile to open a PR. otherwise we can leave it as is.

A PR or even patches would be apreciated.

Matt: Sorry I just realized I could have applied your comment to this update. I will make sure to check those before committing though.

[amehsan]

2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

Maybe I am missing something, but this seems different from (1). Note that addPostRASched is called from addMachinePasses which is not typically overriden by targets. SystemZ has a usecase for targetSchedulesPostRAScheduling which cannot be covered by (1).

[amehsan]

I opened a PR for this and copied the last couple of comments there. CC'ed Eric and Matt on the PR.

[echristo]

lib/CodeGen/TargetPassConfig.cpp

541–542

To elaborate more on this, I'm not sure we want to have the technical debt of even more ways to deal with this than we already have. I'm pretty unhappy with the existing aspects :)

[MatzeB]

In D25482#585479, @amehsan wrote:

2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

Maybe I am missing something, but this seems different from (1). Note that addPostRASched is called from addMachinePasses which is not typically overriden by targets. SystemZ has a usecase for targetSchedulesPostRAScheduling which cannot be covered by (1).

Overriding addPostRASched() with an empty function has the same effect as returning true in targetSchedulesPostRAScheduling(), hasn't it?

[amehsan]

In D25482#585490, @MatzeB wrote:

In D25482#585479, @amehsan wrote:

2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

Maybe I am missing something, but this seems different from (1). Note that addPostRASched is called from addMachinePasses which is not typically overriden by targets. SystemZ has a usecase for targetSchedulesPostRAScheduling which cannot be covered by (1).

Overriding addPostRASched() with an empty function has the same effect as returning true in targetSchedulesPostRAScheduling(), hasn't it?

Correct :)

[MatzeB]

In D25482#585491, @amehsan wrote:

In D25482#585490, @MatzeB wrote:

In D25482#585479, @amehsan wrote:

2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

Maybe I am missing something, but this seems different from (1). Note that addPostRASched is called from addMachinePasses which is not typically overriden by targets. SystemZ has a usecase for targetSchedulesPostRAScheduling which cannot be covered by (1).

Overriding addPostRASched() with an empty function has the same effect as returning true in targetSchedulesPostRAScheduling(), hasn't it?

Correct :)

So we can remove targetSchedulesPostRAScheduling() and replace it with the empty function overload after adding addPostRASched().

[amehsan]

In D25482#585510, @MatzeB wrote:

In D25482#585491, @amehsan wrote:

In D25482#585490, @MatzeB wrote:

In D25482#585479, @amehsan wrote:

2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

Maybe I am missing something, but this seems different from (1). Note that addPostRASched is called from addMachinePasses which is not typically overriden by targets. SystemZ has a usecase for targetSchedulesPostRAScheduling which cannot be covered by (1).

Overriding addPostRASched() with an empty function has the same effect as returning true in targetSchedulesPostRAScheduling(), hasn't it?

Correct :)

So we can remove targetSchedulesPostRAScheduling() and replace it with the empty function overload after adding addPostRASched().

That's a reasonable change for this patch. I'll wait to see Eric's comments well, before updating the patch though. Just to save time.

[amehsan]

I put this back in review as Eric wanted to take a look.

[amehsan]

In D25482#585513, @amehsan wrote:

In D25482#585510, @MatzeB wrote:

In D25482#585491, @amehsan wrote:

In D25482#585490, @MatzeB wrote:

In D25482#585479, @amehsan wrote:

2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

Maybe I am missing something, but this seems different from (1). Note that addPostRASched is called from addMachinePasses which is not typically overriden by targets. SystemZ has a usecase for targetSchedulesPostRAScheduling which cannot be covered by (1).

Overriding addPostRASched() with an empty function has the same effect as returning true in targetSchedulesPostRAScheduling(), hasn't it?

Correct :)

So we can remove targetSchedulesPostRAScheduling() and replace it with the empty function overload after adding addPostRASched().

That's a reasonable change for this patch. I'll wait to see Eric's comments well, before updating the patch though. Just to save time.

Actually let's make this change in a follow up patch. That change will require other reviewers (from SystemZ). I will make it high priority after I commit this one, but let's finish this patch so I have P9 scheduling work checked in.

[MatzeB]

In D25482#589216, @amehsan wrote:

In D25482#585513, @amehsan wrote:

In D25482#585510, @MatzeB wrote:

In D25482#585491, @amehsan wrote:

In D25482#585490, @MatzeB wrote:

In D25482#585479, @amehsan wrote:

2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

Maybe I am missing something, but this seems different from (1). Note that addPostRASched is called from addMachinePasses which is not typically overriden by targets. SystemZ has a usecase for targetSchedulesPostRAScheduling which cannot be covered by (1).

Overriding addPostRASched() with an empty function has the same effect as returning true in targetSchedulesPostRAScheduling(), hasn't it?

Correct :)

So we can remove targetSchedulesPostRAScheduling() and replace it with the empty function overload after adding addPostRASched().

That's a reasonable change for this patch. I'll wait to see Eric's comments well, before updating the patch though. Just to save time.

Actually let's make this change in a follow up patch. That change will require other reviewers (from SystemZ). I will make it high priority after I commit this one, but let's finish this patch so I have P9 scheduling work checked in.

I believe this case is obvious enough that we could go without a review from a SystemZ person (of course if you have some SystemZ people in mind add them as subscribers/reviewers).

[amehsan]

Added the clean up step that we discussed. Also, made some changes in the comments in the code.

[amehsan]

Ping

[echristo]

One inline request for a comment, otherwise LGTM. Thanks for working on this and your patience!

-eric

lib/Target/PowerPC/PPCTargetMachine.cpp

436

Needs a comment as to what's going on here.

[amehsan]

@echristo explicitly approved this in his last comment, but I think he forgot to change the action box. So I accept just for easier book-keeping. (Eric is in vacation, AFAIK)

[echristo]

Relatedly I think we're ok with this for now.

[echristo]

Ehsan has left the building so I'm going to take this over and get it committed.

include/llvm/CodeGen/Passes.h

 #define LLVM_CODEGEN_PASSES_H
 
 #include <functional>
 #include <string>
 
 namespace llvm {
 
 class Function;
+class MachineFunction;
 class FunctionPass;
 class MachineFunctionPass;
 class ModulePass;
 class Pass;
 class TargetMachine;
 class TargetRegisterClass;
 class raw_ostream;
 
/// MachineDominanaceFrontier - This pass is a machine dominators analysis pass.
   extern char &RegisterCoalescerID;
 
   /// MachineScheduler - This pass schedules machine instructions.
   extern char &MachineSchedulerID;
 
   /// PostMachineScheduler - This pass schedules machine instructions postRA.
   extern char &PostMachineSchedulerID;
 
+  MachineFunctionPass *
+  createPostMachineScheduler(std::function<bool(const MachineFunction &)>);
   /// SpillPlacement analysis. Suggest optimal placement of spill code between
   /// basic blocks.
   extern char &SpillPlacementID;
 
   /// ShrinkWrap pass. Look for the best place to insert save and restore
   // instruction and update the MachineFunctionInfo with that information.
   extern char &ShrinkWrapID;
 
/// MachineDominanaceFrontier - This pass is a machine dominators analysis pass.
   /// createPostRAHazardRecognizer - This pass runs the post-ra hazard
   /// recognizer.
   extern char &PostRAHazardRecognizerID;
 
   /// createPostRAScheduler - This pass performs post register allocation
   /// scheduling.
   extern char &PostRASchedulerID;
 
+  MachineFunctionPass *
+  createPostRAScheduler(std::function<bool(const MachineFunction &)>);
   /// BranchFolding - This pass performs machine code CFG based
   /// optimizations to delete branches to branches, eliminate branches to
   /// successor blocks (creating fall throughs), and eliminating branches over
   /// branches.
   extern char &BranchFolderPassID;
 
   /// BranchRelaxation - This pass replaces branches that need to jump further
   /// than is supported by a branch instruction.

include/llvm/CodeGen/TargetPassConfig.h

public:
   /// instructions, thereby constraining all generic virtual registers to
   /// register classes.
   virtual bool addGlobalInstructionSelect() { return true; }
 
   /// Add the complete, standard set of LLVM CodeGen passes.
   /// Fully developed targets will not generally override this.
   virtual void addMachinePasses();
 
+  /// Add scheduler passes to be executed after register allocation.
+  /// Target can optionally insert this pass by itself at some other point by
+  /// overriding this with an empty function.
+  virtual void addPostRASched();
+
   /// Create an instance of ScheduleDAGInstrs to be run within the standard
   /// MachineScheduler pass for this function and target at the current
   /// optimization level.
   ///
   /// This can also be used to plug a new MachineSchedStrategy into an instance
   /// of the standard ScheduleDAGMI:
   ///   return new ScheduleDAGMI(C, make_unique<MyStrategy>(C), /*RemoveKillFlags=*/false)
   ///
protected:
   ///                  machine printer pass afterwards
   /// @p verifyAfter   if true and adding a machine function pass add an extra
   ///                  machine verification pass afterwards.
   void addPass(Pass *P, bool verifyAfter = true, bool printAfter = true);
 
   /// addMachinePasses helper to create the target-selected or overriden
   /// regalloc pass.
   FunctionPass *createRegAllocPass(bool Optimized);
+
+  /// Allow standard passes to be disabled by the command line, regardless of
+  /// who is adding the pass.
+  ///
+  /// StandardID is the pass identified in the standard pass pipeline and
+  /// provided
+  /// to addPass(). It may be a target-specific ID in the case that the target
+  /// directly adds its own pass, but in that case we harmlessly fall through.
+  ///
+  /// TargetID is the pass that the target has configured to override
+  /// StandardID.
+  ///
+  /// StandardID may be a pseudo ID. In that case TargetID is the name of the
+  /// real
+  /// pass to run. This allows multiple options to control a single pass
+  /// depending
+  /// on where in the pipeline that pass is added.
+  virtual IdentifyingPassPtr overridePass(AnalysisID StandardID,
+                                          IdentifyingPassPtr TargetID) const;
+
+  /// Allow standard passes to be disabled by command line options. This
+  /// supports simple binary flags that either suppress the pass or do nothing.
+  /// i.e. -disable-mypass=false has no effect.
+  /// These should be converted to boolOrDefault in order to use applyOverride.
+  IdentifyingPassPtr applyDisable(IdentifyingPassPtr PassID,
+                                  bool Override) const;
 };
 
 } // end namespace llvm
 
 #endif

include/llvm/Target/TargetMachine.h

public:
   /// MCStreamer.
   ///
   virtual bool addPassesToEmitMC(PassManagerBase &, MCContext *&,
                                  raw_pwrite_stream &,
                                  bool /*DisableVerify*/ = true) {
     return true;
   }
 
-  /// True if subtarget inserts the final scheduling pass on its own.
-  ///
-  /// Branch relaxation, which must happen after block placement, can
-  /// on some targets (e.g. SystemZ) expose additional post-RA
-  /// scheduling opportunities.
-  virtual bool targetSchedulesPostRAScheduling() const { return false; };
-
   void getNameWithPrefix(SmallVectorImpl<char> &Name, const GlobalValue *GV,
                          Mangler &Mang, bool MayAlwaysUsePrivate = false) const;
   MCSymbol *getSymbol(const GlobalValue *GV, Mangler &Mang) const;
 
   /// True if the target uses physical regs at Prolog/Epilog insertion
   /// time. If true (most machines), all vregs must be allocated before
   /// PEI. If false (virtual-register machines), then callee-save register
   /// spilling and scavenging are not needed or used.

lib/CodeGen/MachineScheduler.cpp

public:
   static char ID; // Class identification, replacement for typeinfo
 
 protected:
   ScheduleDAGInstrs *createMachineScheduler();
 };
 
 /// PostMachineScheduler runs after shortly before code emission.
 class PostMachineScheduler : public MachineSchedulerBase {
+
+  std::function<bool(const MachineFunction &)> PredicateFtor;
+
 public:
-  PostMachineScheduler();
+  PostMachineScheduler(
+      std::function<bool(const MachineFunction &)> Ftor = nullptr);
 
   void getAnalysisUsage(AnalysisUsage &AU) const override;
 
   bool runOnMachineFunction(MachineFunction&) override;
 
   static char ID; // Class identification, replacement for typeinfo
 
 protected:

 
 char PostMachineScheduler::ID = 0;
 
 char &llvm::PostMachineSchedulerID = PostMachineScheduler::ID;
 
 INITIALIZE_PASS(PostMachineScheduler, "postmisched",
                 "PostRA Machine Instruction Scheduler", false, false)
 
-PostMachineScheduler::PostMachineScheduler()
-: MachineSchedulerBase(ID) {
+PostMachineScheduler::PostMachineScheduler(
+    std::function<bool(const MachineFunction &)> Ftor)
+    : MachineSchedulerBase(ID), PredicateFtor(Ftor) {
   initializePostMachineSchedulerPass(*PassRegistry::getPassRegistry());
 }
 
 void PostMachineScheduler::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.setPreservesCFG();
   AU.addRequiredID(MachineDominatorsID);
   AU.addRequired<MachineLoopInfo>();
   AU.addRequired<TargetPassConfig>();
bool MachineScheduler::runOnMachineFunction(MachineFunction &mf) {
 
   DEBUG(LIS->dump());
   if (VerifyScheduling)
     MF->verify(this, "After machine scheduling.");
   return true;
 }
 
 bool PostMachineScheduler::runOnMachineFunction(MachineFunction &mf) {
+
+  if (PredicateFtor && !PredicateFtor(mf))
+    return false;
+
   if (skipFunction(*mf.getFunction()))
     return false;
 
   if (EnablePostRAMachineSched.getNumOccurrences()) {
     if (!EnablePostRAMachineSched)
       return false;
   } else if (!mf.getSubtarget().enablePostRAScheduler()) {
     DEBUG(dbgs() << "Subtarget disables post-MI-sched.\n");
errs() << "ScheduleDAGMI::viewGraph is only available in debug builds on "
          << "systems with Graphviz or gv!\n";
 #endif  // NDEBUG
 }
 
 /// Out-of-line implementation with no arguments is handy for gdb.
 void ScheduleDAGMI::viewGraph() {
   viewGraph(getDAGName(), "Scheduling-Units Graph for " + getDAGName());
 }
+
+MachineFunctionPass *llvm::createPostMachineScheduler(
+    std::function<bool(const MachineFunction &)> Ftor) {
+  return new PostMachineScheduler(Ftor);
+}

lib/CodeGen/PostRASchedulerList.cpp

DebugMod("postra-sched-debugmod",
                       cl::init(0), cl::Hidden);
 
 AntiDepBreaker::~AntiDepBreaker() { }
 
 namespace {
   class PostRAScheduler : public MachineFunctionPass {
     const TargetInstrInfo *TII;
     RegisterClassInfo RegClassInfo;
+    std::function<bool(const MachineFunction &)> PredicateFtor;
 
   public:
     static char ID;
-    PostRAScheduler() : MachineFunctionPass(ID) {}
+    PostRAScheduler(std::function<bool(const MachineFunction &)> Ftor = nullptr)
+      : MachineFunctionPass(ID), PredicateFtor(Ftor) {}
 
     void getAnalysisUsage(AnalysisUsage &AU) const override {
       AU.setPreservesCFG();
       AU.addRequired<AAResultsWrapperPass>();
       AU.addRequired<TargetPassConfig>();
       AU.addRequired<MachineDominatorTree>();
       AU.addPreserved<MachineDominatorTree>();
       AU.addRequired<MachineLoopInfo>();
bool PostRAScheduler::enablePostRAScheduler(
   if (EnablePostRAScheduler.getPosition() > 0)
     return EnablePostRAScheduler;
 
   return ST.enablePostRAScheduler() &&
          OptLevel >= ST.getOptLevelToEnablePostRAScheduler();
 }
 
 bool PostRAScheduler::runOnMachineFunction(MachineFunction &Fn) {
+
+  if (PredicateFtor && !PredicateFtor(Fn))
+    return false;
+
   if (skipFunction(*Fn.getFunction()))
     return false;
 
   TII = Fn.getSubtarget().getInstrInfo();
   MachineLoopInfo &MLI = getAnalysis<MachineLoopInfo>();
   AliasAnalysis *AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
   TargetPassConfig *PassConfig = &getAnalysis<TargetPassConfig>();
 
for (std::vector<std::pair<MachineInstr *, MachineInstr *> >::iterator
     std::pair<MachineInstr *, MachineInstr *> P = *std::prev(DI);
     MachineInstr *DbgValue = P.first;
     MachineBasicBlock::iterator OrigPrivMI = P.second;
     BB->splice(++OrigPrivMI, BB, DbgValue);
   }
   DbgValues.clear();
   FirstDbgValue = nullptr;
 }
+
+MachineFunctionPass *
+llvm::createPostRAScheduler(std::function<bool(const MachineFunction &)> Ftor) {
+  return new PostRAScheduler(Ftor);
+}
+

lib/CodeGen/TargetPassConfig.cpp

 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/Instrumentation.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils/SymbolRewriter.h"
 
 using namespace llvm;
 
-static cl::opt<bool> DisablePostRA("disable-post-ra", cl::Hidden,
+cl::opt<bool> DisablePostRA("disable-post-ra", cl::Hidden,
     cl::desc("Disable Post Regalloc"));
 static cl::opt<bool> DisableBranchFold("disable-branch-fold", cl::Hidden,
     cl::desc("Disable branch folding"));
 static cl::opt<bool> DisableTailDuplicate("disable-tail-duplicate", cl::Hidden,
     cl::desc("Disable tail duplication"));
 static cl::opt<bool> DisableEarlyTailDup("disable-early-taildup", cl::Hidden,
     cl::desc("Disable pre-register allocation tail duplication"));
 static cl::opt<bool> DisableBlockPlacement("disable-block-placement",
cl::desc("Enable abort calls when \"global\" instruction selection "
              "fails to lower/select an instruction: 0 disable the abort, "
              "1 enable the abort, and "
              "2 disable the abort but emit a diagnostic on failure"),
     cl::init(1));
 
 // Temporary option to allow experimenting with MachineScheduler as a post-RA
 // scheduler. Targets can "properly" enable this with
 // substitutePass(&PostRASchedulerID, &PostMachineSchedulerID).
-// Targets can return true in targetSchedulesPostRAScheduling() and
-// insert a PostRA scheduling pass wherever it wants.
 cl::opt<bool> MISchedPostRA("misched-postra", cl::Hidden,
   cl::desc("Run MachineScheduler post regalloc (independent of preRA sched)"));
 
 // Experimental option to run live interval analysis early.
 static cl::opt<bool> EarlyLiveIntervals("early-live-intervals", cl::Hidden,
     cl::desc("Run live interval analysis earlier in the pipeline"));
 
 // Experimental option to use CFL-AA in codegen
 enum class CFLAAType { None, Steensgaard, Andersen, Both };
 static cl::opt<CFLAAType> UseCFLAA(
     "use-cfl-aa-in-codegen", cl::init(CFLAAType::None), cl::Hidden,
     cl::desc("Enable the new, experimental CFL alias analysis in CodeGen"),
     cl::values(clEnumValN(CFLAAType::None, "none", "Disable CFL-AA"),
                clEnumValN(CFLAAType::Steensgaard, "steens",
                           "Enable unification-based CFL-AA"),
                clEnumValN(CFLAAType::Andersen, "anders",
                           "Enable inclusion-based CFL-AA"),
                clEnumValN(CFLAAType::Both, "both", 
                           "Enable both variants of CFL-AA")));
 
-/// Allow standard passes to be disabled by command line options. This supports
-/// simple binary flags that either suppress the pass or do nothing.
-/// i.e. -disable-mypass=false has no effect.
-/// These should be converted to boolOrDefault in order to use applyOverride.
-static IdentifyingPassPtr applyDisable(IdentifyingPassPtr PassID,
-                                       bool Override) {
-  if (Override)
-    return IdentifyingPassPtr();
-  return PassID;
-}
-
-/// Allow standard passes to be disabled by the command line, regardless of who
-/// is adding the pass.
-///
-/// StandardID is the pass identified in the standard pass pipeline and provided
-/// to addPass(). It may be a target-specific ID in the case that the target
-/// directly adds its own pass, but in that case we harmlessly fall through.
-///
-/// TargetID is the pass that the target has configured to override StandardID.
-///
-/// StandardID may be a pseudo ID. In that case TargetID is the name of the real
-/// pass to run. This allows multiple options to control a single pass depending
-/// on where in the pipeline that pass is added.
-static IdentifyingPassPtr overridePass(AnalysisID StandardID,
-                                       IdentifyingPassPtr TargetID) {
-  if (StandardID == &PostRASchedulerID)
-    return applyDisable(TargetID, DisablePostRA);
-
-  if (StandardID == &BranchFolderPassID)
-    return applyDisable(TargetID, DisableBranchFold);
 
-  if (StandardID == &TailDuplicateID)
-    return applyDisable(TargetID, DisableTailDuplicate);
-
-  if (StandardID == &TargetPassConfig::EarlyTailDuplicateID)
-    return applyDisable(TargetID, DisableEarlyTailDup);
-
-  if (StandardID == &MachineBlockPlacementID)
-    return applyDisable(TargetID, DisableBlockPlacement);
-
-  if (StandardID == &StackSlotColoringID)
-    return applyDisable(TargetID, DisableSSC);
-
-  if (StandardID == &DeadMachineInstructionElimID)
-    return applyDisable(TargetID, DisableMachineDCE);
-
-  if (StandardID == &EarlyIfConverterID)
-    return applyDisable(TargetID, DisableEarlyIfConversion);
-
-  if (StandardID == &MachineLICMID)
-    return applyDisable(TargetID, DisableMachineLICM);
-
-  if (StandardID == &MachineCSEID)
-    return applyDisable(TargetID, DisableMachineCSE);
-
-  if (StandardID == &TargetPassConfig::PostRAMachineLICMID)
-    return applyDisable(TargetID, DisablePostRAMachineLICM);
-
-  if (StandardID == &MachineSinkingID)
-    return applyDisable(TargetID, DisableMachineSink);
-
-  if (StandardID == &MachineCopyPropagationID)
-    return applyDisable(TargetID, DisableCopyProp);
-
-  return TargetID;
-}
 
 //===---------------------------------------------------------------------===//
 /// TargetPassConfig
 //===---------------------------------------------------------------------===//
 
 INITIALIZE_PASS(TargetPassConfig, "targetpassconfig",
                 "Target Pass Configuration", false, false)
 char TargetPassConfig::ID = 0;

 
 bool TargetPassConfig::isPassSubstitutedOrOverridden(AnalysisID ID) const {
   IdentifyingPassPtr TargetID = getPassSubstitution(ID);
   IdentifyingPassPtr FinalPtr = overridePass(ID, TargetID);
   return !FinalPtr.isValid() || FinalPtr.isInstance() ||
       FinalPtr.getID() != ID;
 }
 
+IdentifyingPassPtr TargetPassConfig::applyDisable(IdentifyingPassPtr PassID,
+                                                  bool Override) const {
+  if (Override)
+    return IdentifyingPassPtr();
+  return PassID;
+}
+
+IdentifyingPassPtr TargetPassConfig::overridePass(AnalysisID StandardID,
+                                       IdentifyingPassPtr TargetID) const {
+  if (StandardID == &PostRASchedulerID)
+    return applyDisable(TargetID, DisablePostRA);
+
+  if (StandardID == &BranchFolderPassID)
+    return applyDisable(TargetID, DisableBranchFold);
+
+  if (StandardID == &TailDuplicateID)
+    return applyDisable(TargetID, DisableTailDuplicate);
+
+  if (StandardID == &TargetPassConfig::EarlyTailDuplicateID)
+    return applyDisable(TargetID, DisableEarlyTailDup);
+
+  if (StandardID == &MachineBlockPlacementID)
+    return applyDisable(TargetID, DisableBlockPlacement);
+
+  if (StandardID == &StackSlotColoringID)
+    return applyDisable(TargetID, DisableSSC);
+
+  if (StandardID == &DeadMachineInstructionElimID)
+    return applyDisable(TargetID, DisableMachineDCE);
+
+  if (StandardID == &EarlyIfConverterID)
+    return applyDisable(TargetID, DisableEarlyIfConversion);
+
+  if (StandardID == &MachineLICMID)
+    return applyDisable(TargetID, DisableMachineLICM);
+
+  if (StandardID == &MachineCSEID)
+    return applyDisable(TargetID, DisableMachineCSE);
+
+  if (StandardID == &TargetPassConfig::PostRAMachineLICMID)
+    return applyDisable(TargetID, DisablePostRAMachineLICM);
+
+  if (StandardID == &MachineSinkingID)
+    return applyDisable(TargetID, DisableMachineSink);
+
+  if (StandardID == &MachineCopyPropagationID)
+    return applyDisable(TargetID, DisableCopyProp);
+
+  return TargetID;
+}
+
 /// Add a pass to the PassManager if that pass is supposed to be run.  If the
 /// Started/Stopped flags indicate either that the compilation should start at
 /// a later pass or that it should stop after an earlier pass, then do not add
 /// the pass.  Finally, compare the current pass against the StartAfter
 /// and StopAfter options and change the Started/Stopped flags accordingly.
 void TargetPassConfig::addPass(Pass *P, bool verifyAfter, bool printAfter) {
   assert(!Initialized && "PassConfig is immutable");
 
addPass(createPrintFunctionPass(
         dbgs(), "\n\n*** Final LLVM Code input to ISel ***\n"));
 
   // All passes which modify the LLVM IR are now complete; run the verifier
   // to ensure that the IR is valid.
   if (!DisableVerify)
     addPass(createVerifierPass());
 }
 
+void TargetPassConfig::addPostRASched() {
+  if (getOptLevel() != CodeGenOpt::None) {
+    if (MISchedPostRA)
+      addPass(&PostMachineSchedulerID);
+    else
+      addPass(&PostRASchedulerID);
+  }
+}
+
 /// Add the complete set of target-independent postISel code generator passes.
 ///
 /// This can be read as the standard order of major LLVM CodeGen stages. Stages
 /// with nontrivial configuration or multiple passes are broken out below in
 /// add%Stage routines.
 ///
 /// Any TargetPassConfig::addXX routine may be overriden by the Target. The
 /// addPre/Post methods with empty header implementations allow injecting
void TargetPassConfig::addMachinePasses() {
 
   // Run pre-sched2 passes.
   addPreSched2();
 
   if (EnableImplicitNullChecks)
     addPass(&ImplicitNullChecksID);
 
   // Second pass scheduler.
-  // Let Target optionally insert this pass by itself at some other
-  // point.
-  if (getOptLevel() != CodeGenOpt::None &&
-      !TM->targetSchedulesPostRAScheduling()) {
-    if (MISchedPostRA)
-      addPass(&PostMachineSchedulerID);
-    else
-      addPass(&PostRASchedulerID);
-  }
+  addPostRASched();
 
   // GC
   if (addGCPasses()) {
     if (PrintGCInfo)
       addPass(createGCInfoPrinter(dbgs()), false, false);
   }
 
   // Basic block placement.

lib/Target/PowerPC/PPCTargetMachine.cpp

 #include "llvm/IR/Function.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Target/TargetOptions.h"
 #include "llvm/Transforms/Scalar.h"
+
 using namespace llvm;
 
+extern cl::opt<bool> DisablePostRA;
+
 static cl::
 opt<bool> DisableCTRLoops("disable-ppc-ctrloops", cl::Hidden,
                         cl::desc("Disable CTR loops for PPC"));
 
 static cl::
 opt<bool> DisablePreIncPrep("disable-ppc-preinc-prep", cl::Hidden,
                             cl::desc("Disable PPC loop preinc prep"));
 
public:
   void addIRPasses() override;
   bool addPreISel() override;
   bool addILPOpts() override;
   bool addInstSelector() override;
   void addMachineSSAOptimization() override;
   void addPreRegAlloc() override;
   void addPreSched2() override;
   void addPreEmitPass() override;
+  void addPostRASched() override;
+
+protected:
+  IdentifyingPassPtr overridePass(AnalysisID StandardID,
+                                  IdentifyingPassPtr TargetID) const override;
+
 };
 } // namespace
 
 TargetPassConfig *PPCTargetMachine::createPassConfig(PassManagerBase &PM) {
   return new PPCPassConfig(this, PM);
 }
 
 void PPCPassConfig::addIRPasses() {
void PPCPassConfig::addPreEmitPass() {
   addPass(createPPCBranchSelectionPass(), false);
 }
 
 TargetIRAnalysis PPCTargetMachine::getTargetIRAnalysis() {
   return TargetIRAnalysis([this](const Function &F) {
     return TargetTransformInfo(PPCTTIImpl(this, F));
   });
 }
+
+void PPCPassConfig::addPostRASched() {
+  if (getOptLevel() != CodeGenOpt::None) {
+    if (!isPassSubstitutedOrOverridden(&llvm::PostMachineSchedulerID))
+      addPass(llvm::createPostMachineScheduler([](const MachineFunction &MF) {
+        auto Direc = MF.getSubtarget<PPCSubtarget>().getDarwinDirective();
+        return Direc == PPC::DIR_PWR9;
+      }));
+
+    if (!isPassSubstitutedOrOverridden(&llvm::PostRASchedulerID))
+      addPass(llvm::createPostRAScheduler([](const MachineFunction &MF) {
+        auto Direc = MF.getSubtarget<PPCSubtarget>().getDarwinDirective();
+        return Direc != PPC::DIR_PWR9;
+      }));
+  }
+}
+
+IdentifyingPassPtr
+PPCPassConfig::overridePass(AnalysisID StandardID,
+                            IdentifyingPassPtr TargetID) const {
+  // Since we have two different PostRA schedulers in PPC backend, we need this
+  // extra code here to make sure -diable-post-ra option turns off post ra
+  // scheduling fully even for some special circumstances. For example if user
+  // compiles the code with -mcpu=pwr9 but some functions have target-cpu=pwr8.
+  if (StandardID == &llvm::PostMachineSchedulerID)
+    return applyDisable(TargetID, DisablePostRA);
+
+  return TargetPassConfig::overridePass(StandardID, TargetID);
+}

lib/Target/SystemZ/SystemZTargetMachine.h

public:
   }
   // Override LLVMTargetMachine
   TargetPassConfig *createPassConfig(PassManagerBase &PM) override;
   TargetIRAnalysis getTargetIRAnalysis() override;
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
 
-  bool targetSchedulesPostRAScheduling() const override { return true; };
-
 };
 
 } // end namespace llvm
 
 #endif

lib/Target/SystemZ/SystemZTargetMachine.cpp

createPostMachineScheduler(MachineSchedContext *C) const override {
     return new ScheduleDAGMI(C, make_unique<SystemZPostRASchedStrategy>(C),
                              /*IsPostRA=*/true);
   }
 
   void addIRPasses() override;
   bool addInstSelector() override;
   void addPreSched2() override;
   void addPreEmitPass() override;
+  // Overriding to empty function to schedule post-ra scheduler differently.
+  void addPostRASched() override {};
 };
 } // end anonymous namespace
 
 void SystemZPassConfig::addIRPasses() {
   if (getOptLevel() != CodeGenOpt::None)
     addPass(createSystemZTDCPass());
 
   TargetPassConfig::addIRPasses();