<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><br><div><div>On Feb 1, 2013, at 12:40 AM, Eric Christopher wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">Hi Manman,<div><br></div><div>Looking better. Few more comments/items for discussion:</div><div><br></div><div style="">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.</div>
<div style=""><br></div><div style="">b) Let's go ahead and get a typedef for this:</div><div style=""><br></div><div style=""><div>+ std::map<int, MCLineEntryCollection> MCLineDivisions;</div><div><br></div><div style="">
and then use it.</div><div style=""><br></div><div style="">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).</div><div style=""><br></div><div style="">d) This comment:</div>
<div style=""><br></div><div style=""><div>+ // CUID and MCLineTableSymbols are set in DwarfDebug, when DwarfDebug does</div><div>+ // not exist, CUID will be 0 and MCLineTableSymbols will be empty. </div><div>+ // Handle Compile Unit 0, the line table start symbol is the section symbol.</div>
<div><br></div><div style="">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.</div>
<div style=""><br></div><div style="">e) Similarly instead of this:</div><div style=""><br></div><div style=""><div>+ if (CUID == 0)</div><div>+ // This is the first line table.</div><div>+ LineStartSym = context.CreateTempSymbol();</div>
<div><br></div><div style="">you could set the symbol when you create the rest of them.</div><div style=""><br></div><div style="">f) Similarly again:</div><div style=""><br></div><div style=""><div>+ // Define start line table label for each Compile Unit.</div>
<div>+ MCSymbol *LineTableStartSym = NewCU->getUniqueID() == 0 ?</div><div>+ Asm->GetTempSymbol("section_line") :</div><div>+ Asm->GetTempSymbol("line_table_start",</div>
<div>+ NewCU->getUniqueID());</div><div><br></div><div style="">Go ahead and just use line_table_start for all of the CUs to avoid the extra conditionals.</div><div style="">
<br></div><div style="">g) And then here:</div><div style=""><br></div><div style=""><div>+ else if (NewCU->getUniqueID() == 0)</div><div>+ NewCU->addUInt(Die, dwarf::DW_AT_stmt_list, dwarf::DW_FORM_data4, 0);</div><div>
else</div><div>- NewCU->addUInt(Die, dwarf::DW_AT_stmt_list, dwarf::DW_FORM_data4, 0);</div><div>+ NewCU->addDelta(Die, dwarf::DW_AT_stmt_list, dwarf::DW_FORM_data4,</div><div>+ LineTableStartSym, Asm->GetTempSymbol("section_line"));</div>
<div><br></div><div style="">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.</div><div style=""><br></div><div style="">
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 :)</div>
<div style=""><br></div><div style="">-eric</div></div></div><div><br></div></div></div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jan 31, 2013 at 10:03 AM, Manman Ren <span dir="ltr"><<a href="mailto:mren@apple.com" target="_blank">mren@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div>Patch updated:<div>1> added comments</div><div>2> use 0 as default CUID to simplify logic</div>
<div>3> use std::map and get rid of MCLineIDs</div><div><br></div><div>Please review.</div><div><br></div><div>Thanks,</div><div>Manman</div><div></div></div>
<br><div style="word-wrap:break-word"><div><br><div><div>On Jan 30, 2013, at 5:28 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:</div><br><blockquote type="cite">
<div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>
<blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">Any thoughts here? </div></div></div></blockquote></div>I used MCLineIDs for fast look-up, do you have any suggestion on what to use here?</div>
<div><div><br></div></div></div></blockquote><div><br></div><div>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)?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><blockquote type="cite"><div dir="ltr"><div></div><div>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?</div>
</div></blockquote></div>"-1" is the default CUID, when DwarfDebug does not exist.</div><div>So the first line table will be for CUID 0 when DwarfDebug exists, and will be for CUID -1 when DwarfDebug does not exist.</div>
<div><br></div></div></blockquote><div><br></div><div>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.</div>
</div></div></div></blockquote></div>I added some comments.</div><div><div>+ // CUID and MCLineTableSymbols are set in DwarfDebug, when DwarfDebug does</div><div>+ // not exist, CUID will be -1 and MCLineTableSymbols will be empty. </div>
<div><br></div><div>Updated patch is attached.</div><div><br></div></div></div></blockquote><div><br></div><div>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?</div>
<div><br></div><div>-eric </div></div></div></div>
</blockquote></div><br></div></div>
<br></blockquote></div><br></div>
</blockquote></div><br></div></body></html>