<div dir="ltr">Why do you create the explicit comdat group in ParseValueSymbolTable instead of when you detect the implicit comdat in ParseModule?</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 8, 2015 at 2:12 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Updated patch attached. Just waiting on the review of the clang one.<br>
<br>
On 8 January 2015 at 13:14, Duncan P. N. Exon Smith<br>
<div class="HOEnZb"><div class="h5"><<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
><br>
>> On 2015-Jan-08, at 08:33, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
>><br>
>> On 8 January 2015 at 03:30, David Majnemer <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>> wrote:<br>
>>> I really like this change.  However, don't we need to upgrade existing<br>
>>> bitcode by inserting stuff into newly made comdats?<br>
>><br>
>> I guess so.<br>
>><br>
>> It was not initially clear to me that it would be worth it. Old<br>
>> bitcode files would still work, they would just produce .o files that<br>
>> would not be as optimized by ELF/COFF linkers.<br>
>><br>
>> But the feature is common enough that we should probably upgrade. The<br>
>> attached patch does that.<br>
>><br>
>> Cheers,<br>
>> Rafael<br>
>> <t.patch><br>
><br>
> Aside from a comment nit, this LGTM.  I agree that upgrading makes sense.<br>
><br>
>> diff --git a/lib/Bitcode/Reader/BitcodeReader.cpp b/lib/Bitcode/Reader/BitcodeReader.cpp<br>
>> index 7c7eebd..c9c08c0 100644<br>
>> --- a/lib/Bitcode/Reader/BitcodeReader.cpp<br>
>> +++ b/lib/Bitcode/Reader/BitcodeReader.cpp<br>
>> @@ -100,19 +100,31 @@ static bool ConvertToString(ArrayRef<uint64_t> Record, unsigned Idx,<br>
>>    return false;<br>
>>  }<br>
>><br>
>> +static bool hasImplicitComdat(size_t Val) {<br>
>> +  switch (Val) {<br>
>> +  default:<br>
>> +    return false;<br>
>> +  case 1:  // old WeakAnyLinkage<br>
>> +  case 4:  // old LinkOnceAnyLinkage<br>
>> +  case 10: // old WeakODRLinkage<br>
>> +  case 11: // old LinkOnceODRLinkage<br>
>> +    return true;<br>
>> +  }<br>
>> +}<br>
>> +<br>
>>  static GlobalValue::LinkageTypes getDecodedLinkage(unsigned Val) {<br>
>>    switch (Val) {<br>
>>    default: // Map unknown/new linkages to external<br>
>>    case 0:<br>
>>      return GlobalValue::ExternalLinkage;<br>
>>    case 1:<br>
>> -    return GlobalValue::WeakAnyLinkage;<br>
>> +    return GlobalValue::WeakAnyLinkage; // Old value<br>
><br>
> I think `// Implicit comdat` is better than `// Old value` here.  Or<br>
> maybe `// Old value with implicit comdat`?<br>
><br>
>>    case 2:<br>
>>      return GlobalValue::AppendingLinkage;<br>
>>    case 3:<br>
>>      return GlobalValue::InternalLinkage;<br>
>>    case 4:<br>
>> -    return GlobalValue::LinkOnceAnyLinkage;<br>
>> +    return GlobalValue::LinkOnceAnyLinkage; // Old value<br>
>>    case 5:<br>
>>      return GlobalValue::ExternalLinkage; // Obsolete DLLImportLinkage<br>
>>    case 6:<br>
>> @@ -124,9 +136,9 @@ static GlobalValue::LinkageTypes getDecodedLinkage(unsigned Val) {<br>
>>    case 9:<br>
>>      return GlobalValue::PrivateLinkage;<br>
>>    case 10:<br>
>> -    return GlobalValue::WeakODRLinkage;<br>
>> +    return GlobalValue::WeakODRLinkage; // Old value<br>
>>    case 11:<br>
>> -    return GlobalValue::LinkOnceODRLinkage;<br>
>> +    return GlobalValue::LinkOnceODRLinkage; // Old value<br>
>>    case 12:<br>
>>      return GlobalValue::AvailableExternallyLinkage;<br>
>>    case 13:<br>
>><br>
>> @@ -135,6 +147,14 @@ static GlobalValue::LinkageTypes getDecodedLinkage(unsigned Val) {<br>
>>      return GlobalValue::PrivateLinkage; // Obsolete LinkerPrivateWeakLinkage<br>
>>    case 15:<br>
>>      return GlobalValue::ExternalLinkage; // Obsolete LinkOnceODRAutoHideLinkage<br>
>> +  case 16:<br>
>> +    return GlobalValue::WeakAnyLinkage;<br>
><br>
> Or maybe you should just move it down here:<br>
><br>
>     case 1: // Old value with implicit comdat<br>
>     case 16:<br>
>         return GlobalValue::WeakAnyLinkage;<br>
><br>
>> +  case 17:<br>
>> +    return GlobalValue::WeakODRLinkage;<br>
>> +  case 18:<br>
>> +    return GlobalValue::LinkOnceAnyLinkage;<br>
>> +  case 19:<br>
>> +    return GlobalValue::LinkOnceODRLinkage;<br>
>>    }<br>
>>  }<br>
>><br>
>><br>
><br>
</div></div></blockquote></div><br></div>