[PATCH] Move ownership of GCStrategy objects to LLVMContext

[Philip Reames]

Given you other comment, I'm going to continue this discussion under the 
assumption we're no longer really discussing this patch.  I'll commit a 
cleaned up version of what I have tomorrow and we'll figure out what 
direction we want to move in overall in this thread.  Once we've 
concluded, I'll revise what's in tree.

On 01/15/2015 03:46 PM, Chandler Carruth wrote:
> When I said "construct" I really did just mean "build an instance of 
> one of the known strategies", and was imagining it then being put into 
> the map and re-used everywhere. I think we essentially in agreement 
> about how this works.
Ok.
>
> On Thu, Jan 15, 2015 at 3:23 PM, Philip Reames 
> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>
>         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.
>
>
> Ahh, you want it in the verifier...
>
> So, I'm not sold on this being in the verifier at all. But that seems 
> quite likely a separate discussion.
Why?  Feel free to forward this to llvm-dev depending on the answer and 
it's length.

Being able to state things like: "statepoints in functions with GC X 
only relocate values in an address space X expects" seems to be entirely 
reasonable and desirable to me.

> However, even if it is in the verifier, I think it should still be an 
> analysis pass, and the verifier should explicitly construct it. This 
> is how the verifier uses the dominator trees. The nice thing is that 
> it keeps the verifier from depending on the current state of an 
> analysis pass which might hide actual errors in the IR. So the 
> verifier just directly computes the GCStrategy, and then you have an 
> analysis that provides access to a nicely cached one.
So, first of all, the Verifier does not use the analysis pass 
mechanism.  It natively constructs and recalculates a DominatorTree 
which would normally be obtained through the DomTreeWrapperAnalysis 
pass.  Is this what you meant?

The analogous thing would be a GCStrategyCache and a wrapping 
GCStrategyCacheWrapper pass.  I have no objection to doing this if 
that's what the community would prefer, but I believe this is 
undesirable.  My reasoning is:
1) There is no possibility of invalidation.  A GCStrategy is immutable 
and can only be changed at compile time (of LLVM itself).
2) If we have multiple instances of GCStrategyCache, we have multiple 
copies of each GCStrategy floating around with different lifetimes.  
This seems potentially confusing at best.  (Particularly for any out of 
tree users... I'd really like the lifetime model to be simple.)
3) We're adding an IMHO utterly unnecessary abstraction which only 
increases the complexity of an already fairly complex mechanism.  I see 
no benefit here.
>
>
>         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.
>
>
> Again, I don't really care which order. But I think it would be better 
> to not have this in the Function API, and instead be an analysis over 
> the function.
>
>
>
>         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.
>
>
> One of them was in the changed code which is no longer owning the 
> GCStrategies. For moving code, I'm OK with fixing before or after the 
> move.
Ok.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150115/8e880c21/attachment.html>