[PATCH] D14825: [ThinLTO] Add MODULE_CODE_METADATA_VALUES record

[Mehdi Amini via llvm-commits]

Sent from my iPhone

> On Nov 19, 2015, at 7:26 PM, Teresa Johnson <tejohnson at google.com> wrote:
> 
>> On Thu, Nov 19, 2015 at 3:28 PM, Mehdi AMINI <mehdi.amini at apple.com> wrote:
>> joker.eph accepted this revision.
>> joker.eph added a comment.
>> This revision is now accepted and ready to land.
>> 
>> LGTM (with one question)
>> 
>> 
>> ================
>> Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:1924
>> @@ -1916,1 +1923,3 @@
>>   unsigned NextMDValueNo = MDValueList.size();
>> +  if (ModuleLevel && SeenModuleValuesRecord) {
>> +    // Now that we are parsing the module level metadata, we want to restart
>> ----------------
>> I got a typo in my question, I meant: is it possible to have `NextMDValueNo != 0` and *not* `SeenModuleValuesRecord`?
>> 
>> In the end my question is: do we need to check `SeenModuleValuesRecord` here? Is there a difference between this check and the following:
>> 
>> ```
>> unsigned NextMDValueNo = ModuleLevel ? 0 : MDValueList.size();
> 
> Yes, for very old debug info (the kind we eventually strip via
> StripDebugInfo), I found that there are existing MDValueList entries
> when we come into this routine to parse module-level metadata. I'm not
> sure how the old debug info metadata sections were structured, but I
> don't think we should make any assumptions for old bitcode/debug
> metadata. So I think I should leave the guard around doing this only
> when SeenModuleValuesRecord which guarantees that we are doing this on
> new bitcode. Without this record we can't delay the lazy reading of
> the module-level bitcode until after function-level bitcode anyway, so
> no reset is needed here.
> 

Ok, make sense. Thanks!


Mehdi


> 
>> ```
>> 
>> (Not saying you should write it this way, but what about `if (ModuleLevel) {`)
>> 
>> 
>> 
>> 
>> 
>> http://reviews.llvm.org/D14825
> 
> 
> 
> -- 
> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413