[PATCH] Don't upgrade global constructors when reading bitcode

[Duncan P. N. Exon Smith]

> On 2014-Aug-11, at 16:00, Nick Lewycky <nlewycky at google.com> wrote:
> 
> Sorry for the delay!

Thanks for the review!  I've attached a new copy of the patch, with
answers to your questions inline below.

>> However, the BitcodeReader was changed to always upgrade to the
>> three-field version.  This created an unnecessary inconsistency in the
>> IR before/after serializing to bitcode.
> 
> What problems did this cause? What are you solving?

IMO, writing to bitcode and reading back shouldn't change the IR.

The motivating problem I'm solving is serializing use-list order to
bitcode (PR5680).

> 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:
> 
>   /* requires # ops space for "ops" */
>   void FillFrom(LLVMValueRef Val, LLVMValueRef *ops) {
>     for (int i = 0; i < LLVMGetNumOperands(Val); i++)
>       ops[i] = LLVMGetOperand(Val, i);
>   }
> 
>   [...]
> 
>     LLVMValueRef refs[2];
>     FillOperands(my_global, &refs);
> 
> then they get to notice that we broke our promises.

Does the plan in PR20506 sound reasonable?

> Also we should stop making such promises but that's a discussion in another thread.

Agreed!

>> diff --git a/test/Bitcode/metadata-2.ll b/test/Bitcode/metadata-2.ll
>> index 4055f92..06f8442 100644
>> --- a/test/Bitcode/metadata-2.ll
>> +++ b/test/Bitcode/metadata-2.ll
>> @@ -1,4 +1,5 @@
>>  ; RUN: llvm-as < %s | llvm-dis -disable-output
>> +; RUN: verify-uselistorder < %s -preserve-bc-use-list-order
>>  	%0 = type { %object.ModuleInfo.__vtbl*, i8*, %"byte[]", %1, %"ClassInfo[]", i32, void ()*, void ()*, void ()*, i8*, void ()* }		; type %0
>>  	%1 = type { i64, %object.ModuleInfo* }		; type %1
>>  	%2 = type { i32, void ()* }		; type %2
> 
> Did this creep in from the wrong patch?

No, it's intentional :).  This `verify-uselistorder` invocation fails
prior to the patch and passes after, which is how I came across the
inconsistency.  I've tweaked the commit message to make that more clear.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Don-t-upgrade-global-constructors-when-reading-bit-2.patch
Type: application/octet-stream
Size: 10410 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140811/a3c91f6e/attachment.obj>