<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Aug 19, 2016 at 1:17 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<span><br>
================<br>
Comment at: lib/Transforms/Scalar/LoopSink<wbr>.cpp:133<br>
@@ +132,3 @@<br>
</span>+      INS.push_back(&I);<br>
+  }<br>
+  for (auto I : INS) {<br>
----------------<br>
We need reverse iterator because instructions in the back of the BB may depend on the instructions in the front, thus it needs to be sunk first before other instructions can be sunk.<br></blockquote><div><br></div></span><div>I understand this, i'm asking "why are you using a smallvector and copying them instead of just using reverse iterator directly"?<br></div></div></div></div></blockquote><div><br></div><div>Because the reverse iterator does not support in-place replacement. If I move the current instruction out of the BB, even if I pre-increment the iterator before, the pre-incremented iterator will also be moved to the new BB. I checked around llvm code base, seems there is no in-place update for reverse iterators.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
================<br>
Comment at: lib/Transforms/Scalar/LoopSink<wbr>.cpp:199<br>
@@ +198,3 @@<br>
+<br>
</span>+    int i = 0;<br>
<span>+    for (BasicBlock *N : SinkBBs) {<br>
</span>----------------<br>
SinkBBs.size() == 0 check is already moved above the total frequency check, so it will not reach here.<br>
<br>
The i==0 check is to distinguish between the first SinkBB (that we use move instead of insert) and the later SinkBB (that we make a copy for each insert).</blockquote></span><div><br>Right, i'm just suggesting you move the first SinkBB logic out of the loop becuase it makes the loop a lot easier to understand.</div></div></div></div></blockquote><div><br></div><div>Done.</div><div><br></div><div>Thanks,</div><div>Dehao</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D22778" rel="noreferrer" target="_blank">https://reviews.llvm.org/D2277<wbr>8</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>
</blockquote></div><br></div></div>