This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Feature generic options to setup start/stop-after/before
ClosedPublic

Authored by qcolombet on Mar 13 2017, 2:57 PM.

Details

Summary

Hi,

This patch refactors the code used in llc such that all the users of the addPassesToEmitFile API have access to a homogeneous way of handling start/stop-after/before options right out of the box.

Previously each user would have needed to duplicate this logic and set up its own options.

Note:
I am not super happy about the error reporting. I would have preferred this to be handled by the caller of the getPassXXX methods. However, since I wanted to have the start/stop-after/before options working out-of-the-box, that's not possible. Indeed that would mean that every user of the getPassXXX API would need to have code to unbox/treat the error case. Thus, I settle for a direct reporting. Given this is more of a developer capability, I thought it would be good enough.

Moreover, the current patch loses the prefix "llc: " or "stop-after" in the error message. I initially thought about passing an additional prefix to the error message (with a default "LLVM: "), but again I thought given the usage of that stuff, this was not worth the effort/complexity.

What do people think?

Cheers,
-Quentin

Diff Detail

Repository
rL LLVM

Event Timeline

qcolombet created this revision.Mar 13 2017, 2:57 PM
MatzeB edited edge metadata.Mar 13 2017, 4:49 PM

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

include/llvm/Target/TargetMachine.h
100 ↗(On Diff #91624)

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.

101–108 ↗(On Diff #91624)

Why not just hardcode these names?
(You should be able to use OptionName.ArgStr if you want to reference the name later)

120–169 ↗(On Diff #91624)

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.

tools/llc/llc.cpp
479–485 ↗(On Diff #91624)

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?

509–516 ↗(On Diff #91624)

You should probably move this error checking into TargetMachine as well now.

Hi Matthias,

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

Cheers,
-Quentin

include/llvm/Target/TargetMachine.h
100 ↗(On Diff #91624)

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

101–108 ↗(On Diff #91624)

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.

tools/llc/llc.cpp
479–485 ↗(On Diff #91624)

You mean something like:
getRunPass(StringRef PassName, bool AbortIfNotRegistered, bool HasLimitedPipelineOption)

And going farther, having the AbortXX and HasLimitedXX being field member?

509–516 ↗(On Diff #91624)

I'll update getStartXXID for that.

MatzeB added inline comments.Mar 14 2017, 11:53 AM
include/llvm/Target/TargetMachine.h
100 ↗(On Diff #91624)

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.

tools/llc/llc.cpp
479–485 ↗(On Diff #91624)

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.
davide added a subscriber: davide.May 10 2017, 4:24 PM

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

tools/llc/llc.cpp
479–485 ↗(On Diff #91624)

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.)

  • Move all the logic to handle the options in TargetPassConfig
  • Refactor llc code to avoid code duplication, now only the run-pass pipeline is custom made
  • Add MMI output parameter to be able to parse the module and populate the MachineModuleInfo after creating the pipeline
MatzeB accepted this revision.Jul 27 2017, 2:44 PM

Suggestion for different style below, but I don't really care deeply one way or the other.
Either way this is fine by me to go in; LGTM.

This revision is now accepted and ready to land.Jul 27 2017, 2:44 PM

It would be nice to get this into the 5.0 release, there have been a few times I've needed this functionality for debugging.

Looks like whatever I wrote went missing here it is again:

include/llvm/Target/TargetMachine.h
228–231 ↗(On Diff #108416)

I wonder if instead of setting an external MachineModuleInfo **MMI you could instead use whatever the user passes in and make this a MachineModuleInfo *MMI.

It would be nice to get this into the 5.0 release, there have been a few times I've needed this functionality for debugging.

Note that in the current form it's not magically working for any frontend like you would expect: The MIR printing and verification at the end of a pipeline or loading a MIR file at the beginning of a pipeline is still done in llc even with this patch and would need to replicate that code in other frontends.
(If you're gonna adapt your frontends anyway, then you can do that even without this pass :)

It would be nice to get this into the 5.0 release, there have been a few times I've needed this functionality for debugging.

Note that in the current form it's not magically working for any frontend like you would expect: The MIR printing and verification at the end of a pipeline or loading a MIR file at the beginning of a pipeline is still done in llc even with this patch and would need to replicate that code in other frontends.
(If you're gonna adapt your frontends anyway, then you can do that even without this pass :)

Just reread the code and actually the MIR printing at the end of the pipeline (in -stop-xxx mode) is there.

qcolombet added inline comments.Jul 31 2017, 10:09 AM
include/llvm/Target/TargetMachine.h
228–231 ↗(On Diff #108416)

That would mean that if the users create the information, we won't have a way to get back them something they can use.
Concretely, that would mean we would need to always create the MachineModuleInfo in llc, which is fine.

I don't care either way either :).

That said, I like the fact that this API will make it clear that if you pass something it is used. I'll update and coming hopefully shortly.

This revision was automatically updated to reflect the committed changes.