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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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.
Clear both Actions and OwningActions in Compilation::initCompilationForDiagnostics().
Changed to move ownership into Compilation, as discussed earlier today. Please have a look.
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? |
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". |
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. |