[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 clarifications.
I am ok moving some more error checking within the TargetMachine class, but I have one more question regarding not exposing the API for the options, see my inline comment.

Cheers,
-Quentin



================
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:
> 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.
Ok, but how do we set the default arguments for this API then?

```
TargetMachine ::addPassesToEmitFile
```

Should we just get rid of those arguments and all do that internally?
(Note: I am not a fan of changing the pipeline behind the developer back.)


Repository:
  rL LLVM

https://reviews.llvm.org/D30913