<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 class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 15, 2015 at 3:23 PM, Philip Reames <span dir="ltr"><<a 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. 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><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>