[PATCH] Move ownership of GCStrategy objects to LLVMContext

[Philip Reames]

Minor comments, not ready for re-review.

I've posted a separate review (http://reviews.llvm.org/D7004) for 
removing the last dependency holding GCStrategy in CodeGen.  Once that's 
landed, I'll post an updated diff which will move the entire GCStrategy 
class into IR/*.

Also, I remembered why the analysis pass model wasn't sufficient. You 
can't add a pass dependency for verifyFunction(F) and I want to add 
verification rules based on the property of the GC specified for a 
function.  The simplest example is that gc.root shouldn't appear in a 
function compiled with a gc.statepoint GC and vice versa.


On 01/14/2015 10:39 AM, Philip Reames wrote:
> 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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

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