<div class="gmail_quote"><div>Rafael,</div><div><br></div><div><div>New patch attached. Also, visual diff at:</div></div><div><br></div><div><a href="http://codereview.chromium.org/5119002/" target="_blank">http://codereview.chromium.org/5119002/</a></div>



<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>
</div></div>+  /// \brief Update the layout to coalesce Src into Dst.<br>
+  void CoalesceFragments(MCFragment *Src, MCFragment *Dst);<br>
<br>
Add a note that the actual contents are not copied.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+  if (isFragmentUpToDate(Src)) {<br>
+    if (LastValidFragment == Src)<br>
+      LastValidFragment = Dst;<br>
+    Dst->EffectiveSize += Src->EffectiveSize;<br>
+  } else {<br>
+    // We don't know the effective size of Src, so we have to invalidate Dst.<br>
+    UpdateForSlide(Dst, 0);<br>
+  }<br>
<br>
You have to invalidate Dst in any case, no? Adding something to the<br>
fragment will cause the addresses in fragment following it to change.<br>
This probably works in the current case since we are scanning the<br>
fragments in order.<br>
<br>
Is it too expensive to just call UpdateForSlide for both cases?<br></blockquote><div><br></div><div>Most of the time, nothing ever gets invalidated.</div><div><br></div><div>When two fragments are coalesced, they are combined into one fragment whose total size is equal to the sum of the sizes of the original two. So the addresses of later fragments won't change. Nothing needs to be invalidated unless Dst happens to be the LastValidFragment. (in which case, only Dst is invalidated).</div>









<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+      // Since we may have removed fragments, fix the layout order.<br>
+      it2->setLayoutOrder(FragmentIndex++)<br>
<br>
merged would probably be a better description.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+        it2 = CurDF;<br>
<br>
Setting an iterator in this for loop is a bit strange. Would you mind<br>
removing the ++it2 so that we have a<br>
<br>
if (...)<br>
  it2 = CurDF;<br>
else<br>
 ++it2;<br>
<br>
That way all the iterator updates are close to each other.<br></blockquote><div><br></div><div>I did not change the way this is done, only the name of the variables. The iterator needs to be repositioned since we deleted the instruction fragment it was pointing to. I found the original way to be very clear.</div>




<div><br></div><div>Note that the ++it2 needs to happen in addition to the it2 = CurDF.</div><div>(The next fragment to look at is CurDF+1)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">





<br>
> - David Meyer<br>
<br>
Thanks a lot for working on this. Hopefully it will close some of the<br>
performance gap of "llc -filetype=obj" X "llc && llvm-mc".<br></blockquote><div><br></div><div>I ran my own test on a 1,000,000 line .s file ... there was too much random variance to measure a statistically significant difference. The assembler is already so fast... the greatest advantage of this patch may be reduced memory usage.</div>



<div><br></div><div>Thanks,</div><div>




 - David Meyer</div></div>