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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Looks good in general. But I think this can be done a bit more aggressively with less public API.



================
Comment at: include/llvm/Target/TargetMachine.h:100
+  /// parameters of addPassesToEmitFile.
+  /// @{
+  /// Name of the commandline option to set the StartAfter parameter.
----------------
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.


================
Comment at: include/llvm/Target/TargetMachine.h:101-108
+  /// Name of the commandline option to set the StartAfter parameter.
+  static const char *StartAfterOptName;
+  /// Name of the commandline option to set the StartAfter parameter.
+  static const char *StartBeforeOptName;
+  /// Name of the commandline option to set the StopAfter parameter.
+  static const char *StopAfterOptName;
+  /// Name of the commandline option to set the StopBefore parameter.
----------------
Why not just hardcode these names?
(You should be able to use OptionName.ArgStr if you want to reference the name later)


================
Comment at: include/llvm/Target/TargetMachine.h:120-169
+  /// Helper method to get the pass ID of the Start/Stop After/Before
+  /// passes from the generic options.
+  /// \p Kind defines which pass ID we look for.
+  ///
+  /// This uses getPassID with the value of the related option as
+  /// PassName. In other words, \see getPassID for the usage of
+  /// \p AbortIfNotRegistered.
----------------
Do we really need all of this API? I think this would work just as well if you just put it inside TargetMachine.cpp now (possibly with a single `hasLimitedPpipelineOption()` call (see below) but not more.


================
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) {
----------------
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?


================
Comment at: tools/llc/llc.cpp:509-516
       if (StartBeforeID && StartAfterID) {
         errs() << argv[0] << ": -start-before and -start-after specified!\n";
         return 1;
       }
       if (StopBeforeID && StopAfterID) {
         errs() << argv[0] << ": -stop-before and -stop-after specified!\n";
         return 1;
----------------
You should probably move this error checking into TargetMachine as well now.


Repository:
  rL LLVM

https://reviews.llvm.org/D30913