Page MenuHomePhabricator

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

Authored by echristo on Oct 11 2016, 11:37 AM.

Details

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....)

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
MatzeB added inline comments.Oct 17 2016, 11:39 AM
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).

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

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.

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.

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.

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 updated this revision to Diff 75728.EditedOct 25 2016, 10:35 AM
amehsan edited edge metadata.

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.

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 added a comment.EditedOct 25 2016, 12:55 PM

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.

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 :)

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 edited edge metadata.Oct 25 2016, 6:34 PM

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.

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

Opened a PR for this

kbarton edited edge metadata.Oct 27 2016, 12:40 PM

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.

@MatzeB
Do you have further comments on this?

echristo added inline comments.Oct 27 2016, 4:40 PM
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 added inline comments.Oct 27 2016, 6:07 PM
lib/CodeGen/MachineFunctionPass.cpp
94–98 ↗(On Diff #75728)

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

echristo added inline comments.Oct 28 2016, 12:17 AM
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 added inline comments.Oct 28 2016, 8:48 AM
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 added inline comments.Oct 28 2016, 1:32 PM
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 added inline comments.Oct 29 2016, 10:23 AM
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 added inline comments.Oct 29 2016, 11:00 AM
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 added inline comments.Oct 29 2016, 11:06 AM
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 added inline comments.Oct 30 2016, 7:09 PM
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 added inline comments.Oct 30 2016, 7:37 PM
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 added inline comments.Oct 30 2016, 7:56 PM
lib/CodeGen/MachineFunctionPass.cpp
94–98 ↗(On Diff #75728)

OK. I will update the patch.

amehsan updated this revision to Diff 76497.Oct 31 2016, 4:00 PM
amehsan edited edge metadata.
amehsan added inline comments.Oct 31 2016, 4:03 PM
lib/Target/PowerPC/PPCTargetMachine.cpp
448

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

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 added inline comments.Oct 31 2016, 4:17 PM
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 added inline comments.Nov 1 2016, 7:42 AM
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 added inline comments.Nov 1 2016, 9:24 AM
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)

amehsan updated this revision to Diff 76589.Nov 1 2016, 10:15 AM
MatzeB accepted this revision.Nov 1 2016, 10:23 AM
MatzeB edited edge metadata.

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.

This revision is now accepted and ready to land.Nov 1 2016, 10:23 AM
amehsan updated this revision to Diff 76605.Nov 1 2016, 11:18 AM
amehsan edited edge metadata.

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 added inline comments.Nov 1 2016, 1:16 PM
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 added a comment.Nov 1 2016, 2:17 PM

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

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.

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 added a comment.EditedNov 1 2016, 6:24 PM

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

echristo added inline comments.Nov 1 2016, 6:25 PM
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 added a comment.Nov 1 2016, 6:28 PM

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?

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 added a comment.Nov 1 2016, 6:39 PM

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().

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 requested a review of this revision.Nov 3 2016, 8:41 AM
amehsan edited edge metadata.

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

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.

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 updated this revision to Diff 79767.Nov 30 2016, 9:33 AM
amehsan edited edge metadata.

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

echristo edited edge metadata.Dec 9 2016, 6:44 PM

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 accepted this revision.Dec 19 2016, 11:54 AM
amehsan added a reviewer: 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)

This revision is now accepted and ready to land.Dec 19 2016, 11:54 AM
echristo accepted this revision.Jan 4 2017, 4:35 PM
echristo edited edge metadata.

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

echristo commandeered this revision.Mar 30 2017, 7:26 PM
echristo removed a reviewer: echristo.

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