Page MenuHomePhabricator

Move ownership of Action objects into Compilation.
ClosedPublic

Authored by jlebar on Jan 5 2016, 6:26 PM.

Details

Summary

Previously, Actions owned their inputs. Now the Compilation object owns all Actions. This makes constructing Action graphs which are DAGs much simpler. It also just simplifies in general the ownership semantics of Actions.

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar updated this revision to Diff 44077.Jan 5 2016, 6:26 PM
jlebar retitled this revision from to Switch Action and ActionList over to using std::shared_ptr..
jlebar updated this object.
jlebar added a reviewer: echristo.
jlebar added a subscriber: cfe-commits.

The main reason I want this is for CUDA. The way CUDA compilation will work, once I finish my patch, is: For each GPU arch, we compile device code to assembly (ptx) and then assemble the ptx into an object file (cubin). We then pass the cubins *and* ptx files to nVidia's fatbinary program, which "links" them into one blob. We then embed the blob in the host code.

So in this scheme, our Action graph is a DAG: The ptx is used as an input both to the cubin and fatbin actions. Making this DAG work in the absence of this patch is...pretty unpleasant.

tra added a subscriber: tra.Jan 7 2016, 11:51 AM
tra added inline comments.
lib/Driver/Action.cpp
66 ↗(On Diff #44077)

I guess we no longer need this destructor.

lib/Driver/Driver.cpp
1053 ↗(On Diff #44077)

unneeded {}

jlebar updated this revision to Diff 44272.Jan 7 2016, 3:31 PM
jlebar marked 2 inline comments as done.

Address review comments.

jlebar updated this revision to Diff 44307.Jan 7 2016, 7:18 PM

Switch to maintaining ownership of the Actions within the Compilation.

jlebar retitled this revision from Switch Action and ActionList over to using std::shared_ptr. to Move ownership of Action objects into Compilation. .Jan 7 2016, 7:20 PM
jlebar updated this object.
jlebar removed a reviewer: echristo.Jan 7 2016, 7:26 PM
jlebar added a subscriber: echristo.
jlebar updated this revision to Diff 44308.Jan 7 2016, 7:29 PM
jlebar retitled this revision from Move ownership of Action objects into Compilation. to Move ownership of Action objects into Compilation..

Clear both Actions and OwningActions in Compilation::initCompilationForDiagnostics().

jlebar removed a subscriber: echristo.

Changed to move ownership into Compilation, as discussed earlier today. Please have a look.

jlebar added a subscriber: dblaikie.Jan 7 2016, 7:32 PM
tra added inline comments.Jan 8 2016, 10:08 AM
include/clang/Driver/Action.h
36 ↗(On Diff #44308)

There's no API to pass ownership to Compilation explicitly, so the only way for an Action to be owned by Compilation is to create it with MakeAction.

Perhaps "Actions created with MakeAction<>() are owned by Compilation"

BTW, should we (can we?) make MakeAction<>() the only way to create actions?

dblaikie added inline comments.Jan 8 2016, 2:07 PM
include/clang/Driver/Compilation.h
54 ↗(On Diff #44308)

Seems like this name isn't quite right ("OwningActions" sounds like "these are the actions that own <something>") - perhaps "AllActions"? (maybe "ActionHolder"? Not sure)

118 ↗(On Diff #44308)

Did you find this static_assert to be particularly helpful? Similar errors (though somewhat more verbose) would be produced during the push_back call a few lines down anyway.

120 ↗(On Diff #44308)

Up to you, but we usually just drop the "std::unique_ptr<T>" in favor of "auto" when the RHS has "make_unique".

jlebar updated this revision to Diff 44379.Jan 8 2016, 2:33 PM
jlebar marked 4 inline comments as done.

Address tra and dblaikie's review comments.

include/clang/Driver/Action.h
36 ↗(On Diff #44308)

Updated the comment.

include/clang/Driver/Compilation.h
118 ↗(On Diff #44308)

Fair enough. I got rid of it but changed my unique_ptr into a unique_ptr<Action> so that we'll get an error somewhere other than inside the bowels of SmallVector.

120 ↗(On Diff #44308)

Done, but then I realized I can just use operator new and skirt this entirely.

echristo accepted this revision.Jan 11 2016, 2:46 PM
echristo edited edge metadata.

LGTM. Thanks for doing this :)

-eric

This revision is now accepted and ready to land.Jan 11 2016, 2:46 PM
This revision was automatically updated to reflect the committed changes.