<div dir="ltr">Hi Duncan,<div><br></div><div>Few random comments on things, in general looks pretty good.</div><div><br></div><div>0001 - LGTM.</div><div>0002 - LGTM.</div><div>0003 -</div><div><br></div><div><div>   struct AttrEntry {</div><div>     DIEValue Val;</div><div>-    const DIEAbbrevData *Desc;</div><div>   };</div><div><br></div><div>Most boring struct ever?</div><div><br></div><div>+  const DIEAbbrev &Abbrev = getAbbrev(Die.getAbbrevNumber());<br></div><div><br></div><div>This is a bit of an awkward construction. I understand where it's coming from, but can you go back and comment the various abbreviation bits with how it works now?</div><div><br></div><div><div>-  AssignAbbrev(Die->getAbbrev());</div><div>+  AssignAbbrev(NewAbbrev);</div><div>+  Die->setAbbrevNumber(NewAbbrev.getNumber());</div></div><div><br></div><div>To elaborate - the need for this sort of thing should be documented.</div><div><br></div><div>0004 - LGTM.</div><div>0005 - Better commit message saying why you're going to want this more later?</div><div>0006 - LGTM.</div><div>0007 - LGTM, might want to have Dave take a look at it as well.</div><div>0008 - reverseChildren and finalizeChildren is pretty gross. Also, I'm going to punt on the rest of this to Dave.</div><div><br></div><div>Thanks for all the work!</div><div><br></div><div>-eric</div><br><div class="gmail_quote">On Sun, May 24, 2015 at 11:55 AM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On 2015 May 20, at 17:51, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
><br>
><br>
>> On 2015 May 20, at 17:39, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
>><br>
>> With just those four patches, memory usage went *up* slightly.  Add in<br>
>> the 5th patch (which does #2 below), and we get an overall memory drop<br>
>> of 4%.<br>
><br>
> Forgot to post numbers for this.  Peak memory was at 920 MB before<br>
> the five patches, and 884 MB after.  (These exact numbers won't quite<br>
> reproduce in ToT since I still haven't finished cleaning up and<br>
> committing the MCSymbol and emitLabelDiff() work I hacked on top of,<br>
> but the 36 MB drop should hold.)<br>
<br>
I've cleaned all this up and committed the most obvious parts, as well<br>
as a few unrelated memory improvements.  I'm attaching my (almost?)<br>
ready-to-go patches, which have the following effects on peak memory:<br>
<br>
  - 0000: 845 MB (baseline)<br>
  - 0001: 845 MB - refactor<br>
  - 0002: 879 MB - pass DIEValue by value (momentary setback)<br>
  - 0003: 829 MB - merge DIEAbbrevData into DIEValue<br>
  - 0004: 829 MB - refactor<br>
  - 0005: 829 MB - refactor<br>
  - 0006: 829 MB - refactor<br>
  - 0007: 764 MB - change DIE::Values to a linked list<br>
  - 0008: 756 MB - change DIE::Children to a linked list<br>
<br>
(Still measuring memory on `llc` for `-flto -g`; details in r236629.)<br>
<br>
@Eric, you mentioned offline you'd like to have a look at this proposal<br>
before I proceed -- obviously I've been impatient ;).  Let me know if<br>
I'm okay to move forward and start committing (modulo a couple of these<br>
that I'll want a review from David and Fred on).<br>
<br>
</blockquote></div></div></div>