<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 17, 2015 at 3:37 PM, Cong Hou <span dir="ltr"><<a href="mailto:congh@google.com" target="_blank">congh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">congh marked 2 inline comments as done.<br>
<span class=""><br>
================<br>
Comment at: lib/CodeGen/MachineBasicBlock.cpp:587<br>
@@ -575,1 +586,3 @@<br>
<br>
+  // FIXME: Temporarily comment the following code as probabilities are now only<br>
+  // used during instruction lowering, but this interface is called in later<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> congh wrote:<br>
> > davidxl wrote:<br>
> > > Is it better to relax getProbabilityIterator for now to not assert and return end iterator instead?<br>
> > ><br>
> > > Also can the opposite situation happen? The removed old BB does not have an corresponding weight? In that case, it is better to relax the getXXXiterator for now.<br>
> > > Is it better to relax getProbabilityIterator for now to not assert and return end iterator instead?<br>
> ><br>
> > I think the assertion doesn't hurt and can help us capture exceptions. What is the advantage of using an end iterator? You mean to eliminate some branches?<br>
> ><br>
> > ><br>
> > > Also can the opposite situation happen? The removed old BB does not have an corresponding weight? In that case, it is better to relax the getXXXiterator for now.<br>
> ><br>
> > This should not happen: we should always guarantee that the weight list is either empty or has the same size of the successor list.<br>
> What I mean is that with the relaxed check (temporary), you can remove the #if 0 ... #endif hack here.<br>
</span>I am afraid that this could not remove #if 0 ... #endif as the root cause is that probability list and successor list may not be synchronized and here we are always removing a successor using an iterator to successor list.<br>
<span class=""><br></span></blockquote><div><br></div><div>In this case, would the getProbabilityIterator return end/null iterator, which the caller can skip?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
================<br>
Comment at: lib/CodeGen/MachineBasicBlock.cpp:635<br>
@@ -618,3 +634,3 @@<br>
   // Update its weight instead of adding a duplicate edge.<br>
   if (!Weights.empty()) {<br>
     weight_iterator OldWI = getWeightIterator(OldI);<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> congh wrote:<br>
> > davidxl wrote:<br>
> > > The refactored removeSuccessor can call an internal function<br>
> > ><br>
> > > removeSuccessor(succ_iterator OldI, succ_iterator NewI) -- when NewI is end(), do not update NewI's weight/prob.<br>
> > ><br>
> > > replaceSuccessor can then call this method as well.<br>
> > Do you mean replaceSuccessor? If New is not added to the successor list, how to reference it with an iterator?<br>
> I mean make iterator based removeSuccessor interface more general to also take an optional NewI iterator. If NewI is Successors.end(), removeSuccessor behaves the same as the current implementation. If it is a value iterator, removeSuccessor can also be used by replaceSuccessor.<br>
</span>I think it is difficult to let replaceSuccessor() call removeSuccessor(). We have two cases:<br>
<br>
1. New is not a successor of MBB. In this case we replace Old with New without removing any successor.<br>
<br>
2. New is a successor of MBB. In this case we need to update the weight/probability of New. It seems the current code is more efficient: if we call removeSuccessor() to remove Old, we need to find the weight/probability of it before removing it. We don't want to search the weight/probability twice.<br>
<br>
<br></blockquote><div><br></div><div>I am not suggesting replacing replaceSuccessor with removeSuccessor, but just the part that does 1) update weight list, 2) update prob list, and 3) erase oldI from Successors. </div><div><br></div><div>When the code reaches this part, NewI should be valid -- see there is an early return when NewI replaces OldI.  There is also no efficiency issue I see -- OldI and NewI are already computed.</div><div><br></div><div>David</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<a href="http://reviews.llvm.org/D14361" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14361</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>