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



In https://reviews.llvm.org/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.



================
Comment at: include/llvm/CodeGen/MachineFunctionPass.h:34
+
+  static char ID;
+
----------------
MatzeB wrote:
> 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.
We should not need to define an ID for the base class as we never put a bare MachineFunctionPass into the PassManager pipeline.


================
Comment at: include/llvm/CodeGen/MachineFunctionPass.h:70-72
+  virtual char &getPassID() const {
+    return MachineFunctionPass::ID;
+  }
----------------
There is already `AnalysisID getPassID() const` in the base class which does that (just returns a `AnalysisID` pointer instead of a reference).


================
Comment at: include/llvm/CodeGen/MachineFunctionPass.h:74-75
+ 
+  using FunctionPass::skipFunction;
+  bool skipFunction(const MachineFunction &F) const;
 private:
----------------
This is highly confusing to have different behavior based on which overload you choose, there is not even a comment here describing the difference.


================
Comment at: include/llvm/Target/TargetSubtargetInfo.h:225-228
+  /// Allows to check whether a pass should run on this subtarget or not.
+  /// For example different PPC CPUs use different PostRA schedulers and
+  /// each function can override its target-cpu. So each of the PostRASched
+  /// passes should ask the function's subtarget whether they should run or not.
----------------
This does not mention that this will only work if the MachineFunction variant of skipFunction is used.


================
Comment at: lib/Target/PowerPC/PPCSubtarget.cpp:263
+
+  llvm_unreachable("Unknown reason!");
+}
----------------
This cannot be reached and can be removed.


================
Comment at: lib/Target/PowerPC/PPCTargetMachine.cpp:455-456
+    !TM->targetSchedulesPostRAScheduling()) {
+      addPass(&PostMachineSchedulerID);
+      addPass(&PostRASchedulerID);
+  }
----------------
indentation


================
Comment at: lib/Target/PowerPC/PPCTargetMachine.cpp:460-466
+IdentifyingPassPtr PPCPassConfig::overridePass(AnalysisID StandardID,
+                                            IdentifyingPassPtr TargetID) const{
+  if (StandardID == &llvm::PostMachineSchedulerID)
+    return applyDisable(TargetID, DisablePostRA);
+
+  return TargetPassConfig::overridePass(StandardID, TargetID);
+}
----------------
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`.


================
Comment at: lib/Target/PowerPC/PPCTargetMachine.cpp:461
+IdentifyingPassPtr PPCPassConfig::overridePass(AnalysisID StandardID,
+                                            IdentifyingPassPtr TargetID) const{
+  if (StandardID == &llvm::PostMachineSchedulerID)
----------------
indentation, please check clang-format.


https://reviews.llvm.org/D25482