<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 31, 2016 at 11:14 AM, Bjorn Pettersson <span dir="ltr"><<a href="mailto:bjorn.a.pettersson@ericsson.com" target="_blank">bjorn.a.pettersson@ericsson.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">bjope added inline comments.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>Reassociate.cpp:2182-2185<br>
+    // Skip dead basic blocks.<br>
+    if (RankMap.count(&*BI) == 0)<br>
+      continue;<br>
+<br>
----------------<br>
</span><span class="">dberlin wrote:<br>
> davide wrote:<br>
> > You may want to expand this comment a bit.<br>
> There is also a much simpler and more consistent fix:<br>
><br>
> Iterate in the same order rankmap was built.<br>
><br>
> You can share the ReversePostOrder ordering, and use it here, and then both are doing the same thing, so there can't be any mismatches.<br>
><br>
</span>Ok. I have no idea if the iteration order in here in ReassociatePass::run is important or not, so I did not dare to do such a change.</blockquote><div><br></div><div>Better to see, and then do this if we can't change that easily. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I just wanted to correct the bug in easiest way without changing anything else.<br>
<br></blockquote><div>Easiest may not be the best though ;)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I also read somewhere that the ReversePostOrder iteration is very expensive and should be avoided. </blockquote><div><br></div><div>This is a pretty old comment. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">So doing that twice seemed like a bad idea. But maybe your idea was to save the order somehow by adding another data structure?<br></blockquote><div><br></div><div>Yes, just save the mapping, we do this elsewhere.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
If it does not matter in which order the basic blocks are analysed, then I guess it is possible to iterate over the keys in the RankMap map (however, I'm not familiar about the DenseMap iterators so I do not know about if that is cheap, if order is predictable when debugging different builds, etc).<br></blockquote><div><br></div><div>It's not deterministic, so you can't do it. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I still think my solution is a minimal impact bugfix.<br></blockquote><div><br></div><div>I would say it's the easiest. It would be appreciated if you were willing to try to come up with something slightly better.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If someone wants to refactor/optimise etc on this later, then I don't mind.<br>
<br>
<br>
<br>
<br>
<a href="https://reviews.llvm.org/D26154" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D26154</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>