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

[Matthias Braun via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: lib/CodeGen/MachineScheduler.cpp:140
 class PostMachineScheduler : public MachineSchedulerBase {
+
+  std::function<bool(const MachineFunction &)> PredicateFtor;
----------------
We usually don't have an empty line at the beginning of a class.


================
Comment at: lib/CodeGen/MachineScheduler.cpp:367
 bool PostMachineScheduler::runOnMachineFunction(MachineFunction &mf) {
+
+  if (PredicateFtor && !PredicateFtor(mf))
----------------
We usually do no have a newline at the beginning of a function. Same for PostRAScheduler::runOnMachineFunction.


================
Comment at: lib/CodeGen/PostRASchedulerList.cpp:717
+}
+
----------------
No newline at end of file.


================
Comment at: lib/CodeGen/TargetPassConfig.cpp:559-560
+
+  if (getOptLevel() != CodeGenOpt::None &&
+      !TM->targetSchedulesPostRAScheduling()) {
+    if (MISchedPostRA)
----------------
amehsan wrote:
> amehsan wrote:
> > MatzeB wrote:
> > > 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).
> > 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. 
> 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)
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.


================
Comment at: lib/Target/PowerPC/PPCTargetMachine.cpp:305
+                                  IdentifyingPassPtr TargetID) const override;
+
 };
----------------
We usually do not have a newline before the end of a class.


https://reviews.llvm.org/D25482