<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Apr 29, 2014, at 1:00 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 29, 2014 at 11:55 AM, Andrew Trick <span dir="ltr"><<a href="mailto:atrick@apple.com" target="_blank">atrick@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class=""><br>
On Apr 29, 2014, at 11:39 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
<br>
><br>
> On 2014-Apr-28, at 16:08, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
><br>
>> First and foremost, I'm extremely concerned about baking the Pass type into the C API. The on-going work to change how the pass manager works at a fundamental level is going to run completely afoul of this.<br>
><br>
> This is clearly important.<br>
<br>
</div>I don’t understand the issue at all.<br></blockquote><div><br></div><div>So, for starters, no one working on the pass manager was even involved in the design discussion. As a consequence, the design of the C API doesn't really match the design of the pass manager. See below for details.</div>
<div> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto;">
<div class=""><br>
>> I would really like to keep this out of the C API which we have to be backwards compatible with until the pass manager work settles down. If we don't, the entire thing is going to be really challenging to make progress on.<br>
><br>
> If I understand correctly, all that's been exposed in the C API here is<br>
> that there is a type for a Pass, and that there's a way to get a name<br>
> from a Pass.<br>
><br>
> In the new API, are we not going to have a Pass base class?</div></blockquote><div><br></div><div>Correct, there is no longer a base class.</div><div><br></div><div>If you want to expose a type-erased generic interface to all passes in the C API we'll actually need to define that new interface. And I'm somewhat concerned that it will be defined in a way that is quite awkward to implement with the new system. Generally, I've had more success designing the core set of interfaces used in the C++ side, and only after they have stabilized, wrapping them in a C interface.</div>
<div> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto;"><div class="">> Or am I<br>
> missing something else? (Sorry if this is obvious; I haven't spent the<br>
> time to grok the new design yet.)<br>
<br>
</div>Exactly. The C API should be totally independent of PassManager implementation. If we need to to hold up a useful C API because of PassManager changes, then something is wrong with the API—but I’m not sure what that is.<br>
</blockquote><div><br></div><div>If you want the C API to be totally independent of the implementation, you've not succeeded -- you're defining a single opaque base class for a pass (not a pass manager) which is an artifact of the current implementation. It's close though -- you could define a wrapper class instead and then it would work just fine.</div>
<div><br></div><div>However, I'm more worried about the assumptions this is baking into the *interface*. While I'm glad to see that the units of IR are incorporated now, they don't even really match the way we use the current pass manager. Here are my concerns, I'm numbering them to help keep both my thoughts and responses attached to the right topics.</div>
<div><br></div><div>1) You expose the set of IR units which passes can operate on in the number and types of arguments to the C API. This completely breaks extension in the future.</div></div></div></div></blockquote><div><br></div><div>We could make the IR argument opaque and typeless. An additional argument could specify the type of the IR argument. This way existing uses of the C API won’t break with the introduction of an new IR type and</div><div>the IR types could be extended.</div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>2) The list of IR units is simply wrong.</div></div></div></div></blockquote>That could be fixed with what I mentioned above.<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>2a) We don't want to expose basic block passes. We only have four of them, and most are used in the standard compilation model.</div></div></div></div></blockquote>Why artificially put a limit here? The listener interface can also be used by the standard compilation model and would include it in the C interface for completeness.<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>2b) You don't expose the CGSCC passes. The CGSCC pass manager is a fundamental component of the LLVM optimizer. I don't know what use this serves if we can't interact with those.</div></div></div></div></blockquote>Can be added when addressing 1.<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div>2c) You also don't expose the Loop passes. Maybe we shouldn't have them, maybe we should, but today we definitely have them and they are critical.</div></div></div></div></blockquote>Can be added when addressing 1.<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>3) I share Duncan's concerns that it is extremely unclear exactly at what event these callbacks are triggered. The documentation is very sparse there.</div></div></div></div></blockquote><div><br></div><div>Using better function names a Duncan suggested should make this clearer.</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>4) A fundamental aspect of the new design is that analysis passes are cached dynamically so that we can access function analyses from non-function passes (and other benefits, such as not invalidating analyses when passes fail to change anything). I don't understand how these will interact with that model. Perhaps this is just a specific example of #3?</div></div></div></div></blockquote><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>5) How does this work in a world where we parallelize the pass manager? This is particularly concerning as the discussion seems to indicate that there are thread safety guarantees made by the pass manager in its invocation of these callbacks, and I don't know how to parallelize the pass manager and preserve them.</div></div></div></div></blockquote><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div><br></div><div>If email isn't the right forum to have this discussion, I'm happy to use another. But I think these are really important things to figure out before we start baking this stuff into the C API for MCJIT. I would even be interested to see us iterate on the C++ callback API and make progress on implementing it with both pass managers in the tree for some time. Wrapping it in a stable C API seems best after it (along with perhaps the pass manager stuff itself) has stabilized.</div>
</div></div></div>
</blockquote></div><br></body></html>