[PATCH] Move ownership of GCStrategy objects to LLVMContext

[Philip Reames]

On 01/13/2015 06:08 PM, Chandler Carruth wrote:
>
> On Tue, Jan 13, 2015 at 5:58 PM, Philip Reames 
> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>
>     This is pretty much exactly what I have in mind.  I had hoped that
>     this would be suitable as an intermediate step, it sounds like the
>     answer is no.  I'll look at trying to pull GCStrategy further into
>     IR to avoid the lowering issue. If I can get the header across the
>     line, do either of you object if some of the *implementation*
>     stays in CodeGen for the moment?  (As in, for a few days, until I
>     can follow on changes in.)  I'm really trying to avoid one massive
>     change that rips the entire thing apart and puts it back together.
>
>
> This will break builds of folks that don't link CodeGen at all.
I had frankly completely forgotten about this use case.  Do we have an 
in tree example of this?  If I get a clean build, can I be certain I'm 
not breaking these users?

Having said that, I think the move I would actually do would still be 
fine.  The only users of the implementation which would stay 
(temporarily) in CodeGen are in CodeGen themselves.
> Since CodeGen depends on IR already, you can safely do a mechanical 
> move from one library to the other, and then do the refactorings you 
> have in mind if that makes things cleaner for you?
Possibly.  I'll play with this more when I get a chance.  It may take me 
a few days to actually get back to this, the time I'd had to devote to 
this has since mostly disappeared.  And I've missed the 3.6 window, so 
there's really no hurry any more.
>
>
>     Just to be clear, does anyone have concern about the *actual
>     ownership* change?
>
>
> I actually do have some concerns, but I don't really have any good 
> answers yet. Here is my current thinking, which I'm not confident in 
> and may be quite broken:
>
> I feel like this isn't an IR construct so much as a utility for 
> examining or analyzing the IR construct, and the IR construct is just 
> a function attribute. Would it make sense to structure the code that 
> way? I'm imagining a GCStrategyAnalysis and MachineGCStrategyAnalysis 
> or something similar which can operate both at the IR level and at the 
> MI level. But I've not worked enough with GCStrategy to know whether 
> this works, or if it doesn't, why it doesn't and whether the problem 
> is fundamental or fixable.
I would phrase this a bit different (but I'm also still working through 
what exactly GCStrategy was/is/will be).  I view the current GCStrategy 
as being a) a collection of facts about the GC for a particular 
function, b) choices that effect lowering strategy applied and c) the 
actual implementation of gc.root lowering. Currently, all of these apply 
only to MI level transforms, but with gc.statepoint, I want to 
generalize this.  (a) will be used to enable selected optimizations 
based on properties of the GC.  For examples, see the gc.relocate cases 
in InstCombineCalls and their TODOs.  It will also be used to add 
stricter verification in the verifier for gc.statepoint.  (i.e. no 
mixing gc.root and gc.statepoint!).  (b) will still have the same 
purpose, but for both gc.root, the barrier stuff, and gc.statepoint  (c) 
will be split out into it's own set of passes.  Long term, I'd don't 
believe the lowering logic should be part of GCStrategy at all.

(Actually, looking at the code again, the lowering parts are much less 
coupled than I'd remembered.  It might make sense to do that split *first*.)

Currently, there is a GCModuleInfo analysis which could be used to get 
the GCStrategy.  Having to add an analysis to every transform, just to 
get a collection of facts about the GC associated with that function 
seems less than ideal.  I'm not utterly opposed to this approach - i.e. 
I'll do what I have to get things in tree - but this *really* doesn't 
seem like a clean approach to me.  Also, the current GCModuleInfo 
contains details of the gc.root lowering that I want to factor out of 
anything shared by gc.root and gc.statepoint. That's addressable through 
different channels of course.

Overall, my view is that GCStrategy should be a collection of facts 
about a GC.  Each function should have easy access to that collection of 
facts.  The exact mechanism isn't particularly important.

One secondary benefit of having GCStrategy be an IR level object would 
be the possibility to have a new GCStrategy provided from an external 
consumer of the JIT APIs.  I wouldn't say this is a *goal* right now, 
but I'd like to not design it out of existence either. If GCStrategies 
are owned by analysis passes, we'd have to expose a creation callback or 
the registry itself (yuck!).  If GCStrategies are just IR objects, then 
one could simply be added to the LLVMContext during IR generation.

p.s. Thanks for taking the time to think about this.  Having someone to 
discuss with helps clarify things for me and will definitely lead to an 
overall better design.

Philip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150114/28108cc5/attachment.html>