[PATCH] D30913: [NFC] Feature generic options to setup start/stop-after/before

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: include/llvm/Target/TargetMachine.h:100
+  /// parameters of addPassesToEmitFile.
+  /// @{
+  /// Name of the commandline option to set the StartAfter parameter.
----------------
qcolombet wrote:
> MatzeB wrote:
> > We prefer `\{` doxygen commands in llvm instead of `@{`. And from what I remember (though I may be wrong here) you have to use something like `\defgroup` in front for `\{` to have any sensible effect.
> All three are in use at the moment.
> A quick look (grep -c), showed up that @{ is the most used, followed by \{ and then \defgroup.
> Honestly I don't care which one we use, but I'd like to be consistent.
> 
> The coding standard list @{ though:
> http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
Odd that we use @{ but not @param then. Anyway I don't care too much either. \defgroup is needed so doxygen knows what it is you are grouping together with @{. Looking at example it looks like \name works as well, so use that instead of \defgroup.


================
Comment at: tools/llc/llc.cpp:479-485
+    AnalysisID StartBeforeID = TargetMachine::getStartBeforeID();
+    AnalysisID StartAfterID = TargetMachine::getStartAfterID();
+    AnalysisID StopAfterID = TargetMachine::getStopAfterID();
+    AnalysisID StopBeforeID = TargetMachine::getStopBeforeID();
+
     if (!RunPassNames->empty()) {
+      if (StartAfterID || StopAfterID || StartBeforeID || StopBeforeID) {
----------------
qcolombet wrote:
> MatzeB wrote:
> > Maybe keep it simple and move all of this into TargetMachine and just keep a TargetMachine::hasLimitedPipelineOption() or so to check for the start/stop with run-pass error?
> You mean something like:
> getRunPass(StringRef PassName, bool AbortIfNotRegistered, bool HasLimitedPipelineOption)
> 
> And going farther, having the AbortXX and HasLimitedXX being field member?
What I was getting at:

- Currently you expose 7 function and an enum type as API for something that looks like a few commandline options that we should be able to handle inside of TargetMachine.cpp entirely.
- However when you move everything into TargetMachine.cpp we obviously cannot do the
    `if (StartAfterID || StopAfterID || StartBeforeID || StopBeforeID) {` here anymore. That's why I suggested to expose a single function in the API which looks similar to this:
      `class TargetMachine { ... bool hasLimitedPipeline() { return StartAfterID || StopAfterID || StartBeforeID || StopBeforeID; } ... }`
    you could use that here and at that point do not need the {Start|Stop}XXXID variables and all the API to access them anymore.


Repository:
  rL LLVM

https://reviews.llvm.org/D30913