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

Eric Christopher via llvm-commits llvm-commits at lists.llvm.org
Sun Oct 30 19:37:01 PDT 2016


echristo added inline comments.


================
Comment at: lib/CodeGen/MachineFunctionPass.cpp:94-98
+bool MachineFunctionPass::skipFunction(const MachineFunction &MF) const {
+  if (!MF.getSubtarget().runPass(this->getPassID()))
+    return true;
+
+  return FunctionPass::skipFunction(*MF.getFunction());
----------------
amehsan wrote:
> echristo wrote:
> > amehsan wrote:
> > > amehsan wrote:
> > > > echristo wrote:
> > > > > amehsan wrote:
> > > > > > echristo wrote:
> > > > > > > amehsan wrote:
> > > > > > > > echristo wrote:
> > > > > > > > > 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?
> > > > > > > > Because I need MachineFunction's subtarget. Does that answer your question?
> > > > > > > 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.
> > > > > > 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? 
> > > > > 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).
> > > > 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?
> > > 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?//
> > 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?
> 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.
> 
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.


https://reviews.llvm.org/D25482





More information about the llvm-commits mailing list