<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Minor comments, not ready for re-review.<br>
<br>
I've posted a separate review (<a class="moz-txt-link-freetext" href="http://reviews.llvm.org/D7004">http://reviews.llvm.org/D7004</a>) 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/*.<br>
<br>
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. <br>
<br>
<br>
<div class="moz-cite-prefix">On 01/14/2015 10:39 AM, Philip Reames
wrote:<br>
</div>
<blockquote cite="mid:54B6B7DC.3050009@philipreames.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 01/13/2015 06:08 PM, Chandler
Carruth wrote:<br>
</div>
<blockquote
cite="mid:CAGCO0KifP69tk34NCc1nG250=ZfaRQypavoyri2iJE3rZB6q3Q@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra"><br>
<div class="gmail_quote">On Tue, Jan 13, 2015 at 5:58 PM,
Philip Reames <span dir="ltr"><<a
moz-do-not-send="true"
href="mailto:listmail@philipreames.com"
target="_blank">listmail@philipreames.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">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.<br>
</div>
</blockquote>
<div><br>
</div>
<div>This will break builds of folks that don't link
CodeGen at all. </div>
</div>
</div>
</div>
</blockquote>
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?<br>
<br>
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. <br>
<blockquote
cite="mid:CAGCO0KifP69tk34NCc1nG250=ZfaRQypavoyri2iJE3rZB6q3Q@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>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?</div>
</div>
</div>
</div>
</blockquote>
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. <br>
<blockquote
cite="mid:CAGCO0KifP69tk34NCc1nG250=ZfaRQypavoyri2iJE3rZB6q3Q@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> <br>
Just to be clear, does anyone have concern about the
*actual ownership* change?</div>
</blockquote>
</div>
<br>
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:</div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">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.</div>
</div>
</blockquote>
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. <br>
<br>
(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*.)<br>
<br>
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.<br>
<br>
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. <br>
<br>
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. <br>
<br>
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. <br>
<br>
Philip<br>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</body>
</html>