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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Hi Matthias,

Thanks for the quick review.
I'm not sure I got the hasLimitedPipelineOption suggestion. See the inline comment.

Cheers,
-Quentin



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


================
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.
----------------
MatzeB wrote:
> Why not just hardcode these names?
> (You should be able to use OptionName.ArgStr if you want to reference the name later)
I thought it was easier for someone to look them up.
That being said, given I can't put the initializer in the header, that's probably not super useful.
I'll get rid of it.


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


================
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;
----------------
MatzeB wrote:
> You should probably move this error checking into TargetMachine as well now.
I'll update getStartXXID for that.


Repository:
  rL LLVM

https://reviews.llvm.org/D30913