<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
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. <br>
<br>
<div class="moz-cite-prefix">On 01/15/2015 03:46 PM, Chandler
Carruth wrote:<br>
</div>
<blockquote
cite="mid:CAGCO0KgRTki3YGFvuF11NchhXiotCc2Aoe6bodsm98MZPYiBHg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">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.</div>
</div>
</blockquote>
Ok.<br>
<blockquote
cite="mid:CAGCO0KgRTki3YGFvuF11NchhXiotCc2Aoe6bodsm98MZPYiBHg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra"><br>
<div class="gmail_quote">On Thu, Jan 15, 2015 at 3:23 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"><span
class="">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br>
</blockquote>
</span>
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.</blockquote>
<div><br>
</div>
<div>Ahh, you want it in the verifier...</div>
<div><br>
</div>
<div>So, I'm not sold on this being in the verifier at all.
But that seems quite likely a separate discussion. </div>
</div>
</div>
</div>
</blockquote>
Why? Feel free to forward this to llvm-dev depending on the answer
and it's length.<br>
<br>
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. <br>
<br>
<blockquote
cite="mid:CAGCO0KgRTki3YGFvuF11NchhXiotCc2Aoe6bodsm98MZPYiBHg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>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.</div>
</div>
</div>
</div>
</blockquote>
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?<br>
<br>
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:<br>
1) There is no possibility of invalidation. A GCStrategy is
immutable and can only be changed at compile time (of LLVM itself).
<br>
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.)<br>
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. <br>
<blockquote
cite="mid:CAGCO0KgRTki3YGFvuF11NchhXiotCc2Aoe6bodsm98MZPYiBHg@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"><span
class=""><br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
</blockquote>
</span>
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.</blockquote>
<div><br>
</div>
<div>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.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex"><span
class=""><br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also a few trivial comments inline.<br>
</blockquote>
</span>
I will fix these. By preference I'd do them as a follow
up commit since I'm just moving existing code.</blockquote>
</div>
<br>
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.</div>
</div>
</blockquote>
Ok.<br>
<br>
</body>
</html>