<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 11 August 2014 16:36, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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></blockquote><div><br></div><div>It doesn't if you have current IR. Even llvm 1.x did IR auto-upgrading. Sorry, but while what you're suggesting would indeed be a nice property to have, it does not match llvm's established practice. If you want to push for it then the right thing to do is bring a proposal up on llvmdev. I know of one out-of-tree user who deals with us changing the bitcode format by keeping multiple copies of lib/Bitcode around, one for each version that their releases are based off of.<br>

<br></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">The motivating problem I'm solving is serializing use-list order to<br>


bitcode (PR5680).<br>
<div class=""><br>
> 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?<br></blockquote><div><br></div><div>Sounds reasonable to me, what it describes is an ABI break in slow motion with notices and stuff. I'm not sure we have any ABI users who *actually care* which is why I think it's better to move quickly (before we grow any users who do care *and* start relying on the deprecated behaviour), but I'm happy to let you want to handle this however you like.</div>

<div><br></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">
<div class="">> Also we should stop making such promises but that's a discussion in another thread.<br>
<br>
</div>Agreed!<br>
<div class=""><br>
>> diff --git a/test/Bitcode/metadata-2.ll b/test/Bitcode/metadata-2.ll<br>
>> index 4055f92..06f8442 100644<br>
>> --- a/test/Bitcode/metadata-2.ll<br>
>> +++ b/test/Bitcode/metadata-2.ll<br>
>> @@ -1,4 +1,5 @@<br>
>>  ; RUN: llvm-as < %s | llvm-dis -disable-output<br>
>> +; RUN: verify-uselistorder < %s -preserve-bc-use-list-order<br>
>>      %0 = type { %object.ModuleInfo.__vtbl*, i8*, %"byte[]", %1, %"ClassInfo[]", i32, void ()*, void ()*, void ()*, i8*, void ()* }          ; type %0<br>
>>      %1 = type { i64, %object.ModuleInfo* }          ; type %1<br>
>>      %2 = type { i32, void ()* }             ; type %2<br>
><br>
> Did this creep in from the wrong patch?<br>
<br>
</div>No, it's intentional :).  This `verify-uselistorder` invocation fails<br>
prior to the patch and passes after, which is how I came across the<br>
inconsistency.  I've tweaked the commit message to make that more clear.<br></blockquote><div><br></div><div>Got it. I missed that this was about bitcode use list ordering work. Thanks for working on that, by the way! Gnarly problem. Covered in barnacles.</div>

</div><br></div></div>