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

[Ehsan Amiri via llvm-commits]

amehsan added a comment.

In https://reviews.llvm.org/D25482#571885, @MatzeB wrote:

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


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



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


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


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


================
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);
+}
----------------
MatzeB wrote:
> 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`.
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. 


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


https://reviews.llvm.org/D25482