[llvm-commits] [PATCH] Dwarf: support for LTO where a single object file can have multiple line tables

[Manman Ren]

On Feb 1, 2013, at 12:40 AM, Eric Christopher wrote:

> Hi Manman,
> 
> Looking better. Few more comments/items for discussion:
> 
> a) Concerned about the performance difference you mentioned with MCLineDivisions, but for now we can just use the map you've got, I'll experiment with just doing the lookup on the densemap myself.
> 
> b)  Let's go ahead and get a typedef for this:
> 
> +    std::map<int, MCLineEntryCollection> MCLineDivisions;
> 
> and then use it.
Can be done.
> 
> c) Also on that note since CU ids are all non-negative it should be unsigned. (Should fix some casting you had to do as well).
That is true.
> 
> d)  This comment:
> 
> +  // CUID and MCLineTableSymbols are set in DwarfDebug, when DwarfDebug does
> +  // not exist, CUID will be 0 and MCLineTableSymbols will be empty. 
> +  // Handle Compile Unit 0, the line table start symbol is the section symbol.
> 
> I'm not sure there's a reason to handle CU 0 any different than the rest of the CUs is there? It should just be the contents of the line table for that compile unit. Basically trying to unify the handling as much as possible there.
The reason to handle Compile Unit 0 separately is even without DwarfDebug (when MCLineTableSymbols is empty), we need to handle it.
And we then handle the reset by checking the MCLineTableSymbols.
> 
> e)  Similarly instead of this:
> 
> +  if (CUID == 0)
> +    // This is the first line table.
> +    LineStartSym = context.CreateTempSymbol();
> 
> you could set the symbol when you create the rest of them.
This is to generate the same code as before for the case where we have a single compile unit.
> 
> f) Similarly again:
> 
> +  // Define start line table label for each Compile Unit.
> +  MCSymbol *LineTableStartSym = NewCU->getUniqueID() == 0 ?
> +                                Asm->GetTempSymbol("section_line") :
> +                                Asm->GetTempSymbol("line_table_start",
> +                                                   NewCU->getUniqueID());
> 
> Go ahead and just use line_table_start for all of the CUs to avoid the extra conditionals.
This is to generate the same code as before.
We use section_line for the single compile unit before this patch, so I am still using section_line as the start symbol for the first line table.

> 
> g) And then here:
> 
> +  else if (NewCU->getUniqueID() == 0)
> +    NewCU->addUInt(Die, dwarf::DW_AT_stmt_list, dwarf::DW_FORM_data4, 0);
>    else
> -    NewCU->addUInt(Die, dwarf::DW_AT_stmt_list, dwarf::DW_FORM_data4, 0);
> +    NewCU->addDelta(Die, dwarf::DW_AT_stmt_list, dwarf::DW_FORM_data4,
> +                    LineTableStartSym, Asm->GetTempSymbol("section_line"));
> 
> if you save the section line symbol earlier (there's a spot where we save the section symbols we'll use later) then everything can be a label subtraction.
This is also to generate the same code as before. I am not sure whether addDelta("section_line", "section_line") will be the same as addUInt(0).
If it is the same, we can get rid of the "else if" statement.

Once again, thanks for the detailed review.
Please get back to me soon on d,e,f,g :)

Manman
> 
> h) Might be nice to check something with .file/.loc directives (more than test/MC/loc.s) to make sure the line table is being emitted as you'd expect, but that can be a follow on after we get the rest of this done :)
> 
> -eric
> 
> 
> 
> On Thu, Jan 31, 2013 at 10:03 AM, Manman Ren <mren at apple.com> wrote:
> 
> Patch updated:
> 1> added comments
> 2> use 0 as default CUID to simplify logic
> 3> use std::map and get rid of MCLineIDs
> 
> Please review.
> 
> Thanks,
> Manman
> 
> 
> On Jan 30, 2013, at 5:28 PM, Eric Christopher <echristo at gmail.com> wrote:
> 
>> 
>>> Any thoughts here? 
>> I used MCLineIDs for fast look-up, do you have any suggestion on what to use here?
>> 
>> 
>> It took me a while, but I see what you're doing. Effectively I think you can just use a lookup in MCLineDivisionsto see if anything is in the set? Since the SmallSet is going to be stored in exactly the same way anyhow. Also since I've not bothered to look are the CU ids dense or sparse? i.e. 0, 1, 2 or in metadata node order (0, 12, 255, 444)?
>>  
>>>> The bit with CUID = -1 or CUID == 0 seems confusing, can you explain this a bit? At times -1 seems to be for invalid CUs and at other times the first/only CU?
>>> "-1" is the default CUID, when DwarfDebug does not exist.
>>> So the first line table will be for CUID 0 when DwarfDebug exists, and will be for CUID -1 when DwarfDebug does not exist.
>>> 
>>> 
>>> This seems a bit confusing. Since, for example, we emit a CU for assembly files and so something will always be emitting a CU. I could easily not be seeing something though. At worst it really needs some comments.
>> I added some comments.
>> +  // CUID and MCLineTableSymbols are set in DwarfDebug, when DwarfDebug does
>> +  // not exist, CUID will be -1 and MCLineTableSymbols will be empty. 
>> 
>> Updated patch is attached.
>> 
>> 
>> Right. I'm not sure why this division exists. Is there some reason why it's -1 in the other case that I'm missing?
>> 
>> -eric 
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130201/079af8b6/attachment.html>