[PATCH] Move ownership of GCStrategy objects to LLVMContext
listmail at philipreames.com
Thu Jan 15 13:46:43 PST 2015
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
> 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.
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits