This is an archive of the discontinued LLVM Phabricator instance.

Port InstrProfiling pass to new pass manager
ClosedPublic

Authored by davidxl on Mar 12 2016, 5:37 PM.

Details

Summary

When creating the InstrProfiling pass, clang needs to pass the some options to the pass (such as profile output path, etc). There does not seem to be a good way to do that with the new pass manager. Please advise.

Diff Detail

Repository
rL LLVM

Event Timeline

davidxl updated this revision to Diff 50533.Mar 12 2016, 5:37 PM
davidxl retitled this revision from to Port InstrProfiling pass to new pass manager .
davidxl updated this object.
davidxl added a reviewer: chandlerc.
davidxl added a subscriber: llvm-commits.
vsk added a subscriber: vsk.Mar 12 2016, 5:44 PM
chandlerc edited edge metadata.Mar 13 2016, 6:32 AM

See below for how to thread options into a pass with the new pass manager. Note that the passes library that will replace the pass manager builder in the new pass manager may not have the same set of hooks that Clang is using, so there may need to be some new APIs introduced to make this fully functional, but that can be done in follow-up patches.

include/llvm/Transforms/Instrumentation.h
99 ↗(On Diff #50533)

typo: loewring -> lowering

Since you're lifting this into a header, it'd be good to provide some more expansive comments.

lib/Passes/PassRegistry.def
43 ↗(On Diff #50533)

Keep this list sorted?

lib/Transforms/Instrumentation/InstrProfiling.cpp
34–37 ↗(On Diff #50533)

Why not make this the actual new pass manager class? That's how most of the passes with stat work. You can see SROA and recently GVN for examples (admitedly a bit more complex).

This will also show the most basic part of how to parameterize it - the constructors here are exactly what you want to be able to construct this pass out of specific options.

davidxl marked 2 inline comments as done.Mar 13 2016, 10:32 AM
davidxl added inline comments.
lib/Transforms/Instrumentation/InstrProfiling.cpp
34–37 ↗(On Diff #50533)

This can be done -- but it will adds lots of overhead in the new pass manager class itself as this class keeps track of the state that only needed when the pass is actually run. Of course InstrProfOptions need to be part of the pass.

davidxl updated this revision to Diff 50554.Mar 13 2016, 10:33 AM
davidxl edited edge metadata.

Addressed Chandler's review feedbacks.

chandlerc added inline comments.Mar 28 2016, 1:15 PM
lib/Transforms/Instrumentation/InstrProfiling.cpp
34–38 ↗(On Diff #50554)

If you want to create a per-run class that holds the state for each run, that's fine, but I think it would be better done as a separate change.

With the legacy pass manager the state for a single run is already kept inside the pass object and cleared between runs. My suggestion is to start off without changing that design, and if there is a desire to change it, do so as a follow-up change because they are two orthogonal aspects of the design.

davidxl added inline comments.Mar 28 2016, 1:59 PM
lib/Transforms/Instrumentation/InstrProfiling.cpp
34–38 ↗(On Diff #50554)

If you insists this as a separate change, I can do it.

What you said about legacy pass manager is true, but the difference is that with new pass manager, I need to pull lots of implementation details into the common header file which should be avoided. I am not sure about the implication on the design you mentioned here.

davidxl updated this revision to Diff 51940.Mar 29 2016, 9:49 AM

Make InstrProfiling part of the new pass class.

chandlerc requested changes to this revision.Apr 6 2016, 11:40 PM
chandlerc edited edge metadata.
chandlerc added inline comments.
include/llvm/InitializePasses.h
118–126 ↗(On Diff #51940)

Please don't introduce lots of unrelated formatting changes.

include/llvm/LinkAllPasses.h
85–93 ↗(On Diff #51940)

Here too.

include/llvm/Transforms/Instrumentation.h
101 ↗(On Diff #51940)

I wouldn't put all of this into the common Instrumentation.h file. Instead, like with all the other pass ports, please add a header for this specific pass. That may also help address your concerns with the design.

174–177 ↗(On Diff #51940)

My suggestion was not to keep these separate, but to actually have a single class which *is* the new pass manager pass, and can be embedded (like InstrProfiling) inside the legacy pass...

lib/Passes/PassRegistry.def
41 ↗(On Diff #51940)

Pretty sure this should be before "invalidate"?

lib/Transforms/Instrumentation/InstrProfiling.cpp
34 ↗(On Diff #51940)

This isn't a wrapper pass. Those are passes that wrap an analysis result. This is just a transformation pass.

This revision now requires changes to proceed.Apr 6 2016, 11:40 PM
davidxl added inline comments.Apr 7 2016, 1:16 AM
include/llvm/InitializePasses.h
118–126 ↗(On Diff #51940)

hmm, it seems clang-format did that for me.

include/llvm/Transforms/Instrumentation.h
101 ↗(On Diff #51940)

probably only partially address my concern -- there will still be more extra dependency created.

lib/Transforms/Instrumentation/InstrProfiling.cpp
34 ↗(On Diff #51940)

ok.

silvas added a subscriber: silvas.Apr 7 2016, 4:30 PM
silvas added inline comments.
lib/Transforms/Instrumentation/InstrProfiling.cpp
34 ↗(On Diff #51940)

Do we have this documented anywhere? I feel like I would have made the same mistake.

davidxl updated this revision to Diff 53162.Apr 9 2016, 5:12 PM
davidxl edited edge metadata.

Addressed Chandler's review comments.

Note that the creator API name for the legacy pass remains unchanged -- this is to avoid updating Clang. If changing the name is preferred (i.e. adding 'Legacy' in it), it will be done as a follow up.

betulb edited edge metadata.Apr 11 2016, 12:10 AM
betulb added a subscriber: betulb.
davidxl marked an inline comment as done.Apr 13 2016, 9:30 AM

Ping ..

chandlerc accepted this revision.Apr 18 2016, 12:27 AM
chandlerc edited edge metadata.

LGTM with a minor tweak below. Mostly answer questions below.

include/llvm/Transforms/InstrProfiling.h
27 ↗(On Diff #53162)

Probably should be 'class' instead of 'struct' at this point.

38 ↗(On Diff #53162)

Not relevant for this patch, but the use of a typedef struct is really odd in C++ and should be unnecessary. It'd be good to clean this up.

lib/Transforms/Instrumentation/InstrProfiling.cpp
34–36 ↗(On Diff #53162)

No, its just come up in code reviews and examples.

I can try to write some documentation around this.

34–49 ↗(On Diff #53162)

This does force the pass's basic implementation to be in a header, but this usually shouldn't be that big of a deal. If there are really serious issues, the standard techniques for separating it back into a source file can be used. But generally, C++ classes should live in headers and have proper interfaces.

Consider how useful it can be to have these have proper interfaces in header files if you ever want to unit test a pass to exercise behavior not easily reproduced or observed in a full integration test. We've had several places already where the new pass manager interfaces allowed us to test code that would otherwise have been extremely challenging.

This revision is now accepted and ready to land.Apr 18 2016, 12:27 AM

Addressed Chandler's review comments.

Note that the creator API name for the legacy pass remains unchanged -- this is to avoid updating Clang. If changing the name is preferred (i.e. adding 'Legacy' in it), it will be done as a follow up.

Forgot to address this: please feel free to update the API name in this patch, and immediately land a patch to Clang that follows that API update. We routinely do cross-LLVM-project API updates. Just make sure you find the users in Clang or Polly or wherever. It can be very helpful to build and test the updates ahead of time so you can land the patches back to back with a very short delay. The build bots try to wait a few seconds to pick up patches landed in this fashion and avoid spurious failures.

davidxl updated this revision to Diff 54082.Apr 18 2016, 10:51 AM
davidxl edited edge metadata.

Addressed Chandler's review feedback.

This revision was automatically updated to reflect the committed changes.