[PATCH] Move ownership of GCStrategy objects to LLVMContext

[Philip Reames]

On 01/15/2015 03:01 PM, Chandler Carruth wrote:
> Ok, I think this is looking pretty good in general.
Good to hear.  :)
>
> I have one major question left. Does this need to be baked into the IR / context? My *impression* is that the goal here is really just to cache the formation of the GCStrategy object which happens by parsing an attribute. Do I understand this correctly?
I don't think so.  The name of the gc is a key into a map of known 
collector descriptions.  There is no way to generate a "new" strategy 
without modifying code.  Adding such a mechanism would be useful, but is 
an entirely separate discussion.

The fundamental thing I need is a way to access this map from places 
like the Verifier.  I could do this in an uncached way, but that seems 
odd.  (i.e. have a function of GCStrategy which searches the registry 
and creates a new object each time)  Also, it seems potentially 
confusing to have many copies of the same GCStrategy floating around 
with varying lifetimes.

If I'm going to have a cache of GCStrategy objects keyed by their name, 
putting them in LLVMContextImpl seemed like a reasonable place.  I'm 
happy to have them elsewhere, but I need to be able to get to them from 
the Verifier (i.e. outside of a pass manager).

> If so, have you looked at possibly making this a function analysis that has a cache of these which it populates exactly the same way you populate the context? That should still allow you to use essentially the same lookup path here.
My understanding is that this would not work inside the Verifier if used 
via verifyFunction(F).  Is that untrue?  If not, your approach would not 
be sufficient.
> The reason I suggest this approach is that it helps simplify the core IR a bit by letting the IR just deal in an opaque attribute and relying on an analysis to reason about it. I think the code would be almost identical to what you have here just shuffled around a bit. I'd be happy with that either as the first version or with that happening as the immediate follow-up refactoring after this patch.
I would strongly prefer to land this as is, then incrementally improve.  
I've got several changes I'd like to make - which are themselves 
uncontroversial and almost boring - which are blocked by this.
>
> Also a few trivial comments inline.
I will fix these.  By preference I'd do them as a follow up commit since 
I'm just moving existing code.
>
>
> ================
> Comment at: include/llvm/CodeGen/GCMetadata.h:46
> @@ -44,3 +45,3 @@
>     class AsmPrinter;
>     class GCStrategy;
>     class Constant;
> ----------------
> Stale forward declaration.
>
> ================
> Comment at: include/llvm/CodeGen/GCMetadata.h:157-160
> @@ -166,10 +156,6 @@
>     class GCModuleInfo : public ImmutablePass {
> -    typedef StringMap<GCStrategy*> strategy_map_type;
> -    typedef std::vector<std::unique_ptr<GCStrategy>> list_type;
> -
> -    strategy_map_type StrategyMap;
> +    typedef std::vector<GCStrategy*> list_type;
> +    /// A list of GCStrategies which are active in this Module.  These are no
> +    /// longer owning pointers.
>       list_type StrategyList;
>     public:
> ----------------
> I would nuke the typedef (or at least fix its name) at this point.
>
> Also, "no longer" doesn't really make sense in a comment where there is no visible history.
>
> ================
> Comment at: lib/IR/LLVMContextImpl.cpp:183-184
> @@ +182,4 @@
> +
> +  for (GCRegistry::iterator I = GCRegistry::begin(),
> +         E = GCRegistry::end(); I != E; ++I) {
> +    if (Name == I->getName()) {
> ----------------
> Range based loop?
>
> http://reviews.llvm.org/D6811
>
> EMAIL PREFERENCES
>    http://reviews.llvm.org/settings/panel/emailpreferences/
>
>