<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Aug 11, 2014 at 4:36 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On 2014-Aug-11, at 16:00, Nick Lewycky <<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>> wrote:<br>
><br>
> Sorry for the delay!<br>
<br>
Thanks for the review!  I've attached a new copy of the patch, with<br>
answers to your questions inline below.<br>
<div class=""><br>
>> However, the BitcodeReader was changed to always upgrade to the<br>
>> three-field version.  This created an unnecessary inconsistency in the<br>
>> IR before/after serializing to bitcode.<br>
><br>
> What problems did this cause? What are you solving?<br>
<br>
</div>IMO, writing to bitcode and reading back shouldn't change the IR.<br>
<br>
The motivating problem I'm solving is serializing use-list order to<br>
bitcode (PR5680).</blockquote><div><br></div><div>The global_ctors auto-upgrade path is only one of many changes we make to IR when deserializing it, though, so by this argument we shouldn't do any auto-upgrade at all.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
> I'll assume there's a good reason we aren't doing the ideal thing now. We should update to make 3 required and if anyone was using the C API and if anyone's doing something like:<br>
><br>
>   /* requires # ops space for "ops" */<br>
>   void FillFrom(LLVMValueRef Val, LLVMValueRef *ops) {<br>
>     for (int i = 0; i < LLVMGetNumOperands(Val); i++)<br>
>       ops[i] = LLVMGetOperand(Val, i);<br>
>   }<br>
><br>
>   [...]<br>
><br>
>     LLVMValueRef refs[2];<br>
>     FillOperands(my_global, &refs);<br>
><br>
> then they get to notice that we broke our promises.<br>
<br>
</div>Does the plan in PR20506 sound reasonable?</blockquote><div><br></div><div>I think we still don't know what kind of high-level C API we would want that would insulate us from these kinds of changes, and we're unlikely to discover it soon.</div>
<div><br></div><div>If we wait a release cycle and document the change, I don't think anyone will notice, and there will be just as much breakage in six months as there would be now if we stared rejecting the 2-arg form.</div>
<div><br></div><div>I think we can do one of two things:</div><div>1. Keep the third argument as optional indefinitely, and drop the auto-upgrade code.</div><div>2. Make it required starting in 3.5 and document a minor C API break. We would keep the auto-upgrade code for old bitcode.</div>
</div></div></div>