[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 added a comment.

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



================
Comment at: include/llvm/CodeGen/Passes.h:119-120
 
+  MachineFunctionPass *
+  createPostMachineScheduler(std::function<bool(const Function &)>);
   /// SpillPlacement analysis. Suggest optimal placement of spill code between
----------------
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.


================
Comment at: include/llvm/CodeGen/Passes.h:180-181
 
+  MachineFunctionPass *
+  createPostRAScheduler(std::function<bool(const Function &)>);
   /// BranchFolding - This pass performs machine code CFG based
----------------
Same as above, use MachineFunction&


================
Comment at: include/llvm/CodeGen/TargetPassConfig.h:256-257
 
+  // Targets that require specialized method for adding PostRA
+  // scheduling can override this.
+  virtual void addPostRASchedPass();
----------------
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`.


================
Comment at: include/llvm/CodeGen/TargetPassConfig.h:258
+  // scheduling can override this.
+  virtual void addPostRASchedPass();
+
----------------
Rename this to `addPostRASched`, a target could just as well add multiple passes or none at all.


================
Comment at: lib/CodeGen/MachineScheduler.cpp:366-367
+
+  if (PredicateFtor && !PredicateFtor(*mf.getFunction()))
+    return false;
+
----------------
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.


================
Comment at: lib/CodeGen/TargetPassConfig.cpp:275-278
+/// 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.
----------------
This comment should go to TargetPassConfig.h.


================
Comment at: lib/CodeGen/TargetPassConfig.cpp:286-297
+/// 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.
+///
----------------
This comment should go to TargetPassConfig.h.


================
Comment at: lib/CodeGen/TargetPassConfig.cpp:555-557
+  // Second pass scheduler.
+  // Let Target optionally insert this pass by itself at some other
+  // point.
----------------
This comment seems redundant with the doxygen comment for the whole addPostRASchedPass() so we can remove it.


================
Comment at: lib/Target/PowerPC/PPCTargetMachine.cpp:440
+      addPass(llvm::createPostMachineScheduler([this](const Function &F) {
+        auto Direc = this->TM->getSubtarget<PPCSubtarget>(F).getDarwinDirective();
+        return Direc >= PPC::DIR_PWR9 && Direc < PPC::DIR_64;
----------------
This can be simplified to `MF.getSubtarget<PPCSubtarget>().getDarwinDirective()` when the argument is changed to `MachineFunction&` (see above). Similar in the next callback.


https://reviews.llvm.org/D25482