<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 17, 2016 at 2:23 PM, Aditya Kumar <span dir="ltr"><<a href="mailto:hiraditya@msn.com" target="_blank">hiraditya@msn.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">hiraditya added inline comments.<br>
<span class=""><br>
================<br>
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:213<br>
@@ +212,3 @@<br>
+<br>
+ // Return true when all paths from A to the end of the function pass through<br>
+ // either B or C.<br>
----------------<br>
</span><span class="">dberlin wrote:<br>
> Errr, isn't this the definition of A post-dominating B or C?<br>
><br>
><br>
> A post-dominates B if all paths from A to end of function pass through B.<br>
> Same with (A, C).<br>
><br>
> If that's right, i would just use post-dominance here :)<br>
><br>
</span>It is more like B and C combined post dominating A.<br></blockquote><div><br></div><div>Yes, sorry, i reversed it. So why are you not testing that?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
================<br>
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:425<br>
@@ +424,3 @@<br>
+ BasicBlock *BB = Insn->getParent();<br>
+ BasicBlock *NewHoistPt = DT->findNearestCommonDominator(HoistPt, BB);<br>
+ WL.insert(BB);<br>
----------------<br>
</span><span class="">dberlin wrote:<br>
> I wonder how expensive this computation ends up being. It never changes per-iteration unless something is messing with the CFG out from under you.<br>
><br>
> (This is why GCC uses et-splay trees)<br>
</span>For each Instruction in the InstructionsToHoist, the nearest common dominator (w.r.t. HoistPt) could change.<br>
<br>
e.g.,<br>
A -> B -> C (has I1)<br>
B-> D (has I2)<br>
A -> E (has I3).<br>
<br>
And if I1, I2 and I3 have the same GVN.<br>
In this case nearestCommonDominator(C, D) = B, and, nearestCommonDominator(B, E) = A.<br></blockquote><div><br></div><div>True, but my point was that nearestCommonDominator of any two blocks you query it for will not change.</div><div><br></div><div>You are just saying "what we call it on changes". <br>But if you call it repeatedly on the same two blocks, you will allways get the same answer (unless your CFG has changed somehow, and i don't see how that can happen).</div><div><br></div><div>So you could cache NCD into a std::map<std::pair<A,B>, C>.</div><div>:)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
<br>
<br>
================<br>
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:635-637<br>
@@ +634,5 @@<br>
+ continue;<br>
+ if (!OptForMinSize && !Call->onlyReadsMemory())<br>
+ CallWithSideEffect = true;<br>
+ CI.insert(Call, VN);<br>
+ } else if (!CallWithSideEffect && !isa<GetElementPtrInst>(&I1))<br>
----------------<br>
</span><span class="">dberlin wrote:<br>
> majnemer wrote:<br>
> > Likewise, you need to make sure that the call has no side effects which is different from it not mutating memory.<br>
> +1<br>
> Pure and const (in gcc parlance) are pretty much the only thing you can safely move.<br>
><br>
><br>
><br>
</span>Will do that. Thanks.<br>
<span class=""><br>
================<br>
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:665<br>
@@ +664,3 @@<br>
+ for (const BasicBlock *BB : depth_first(&F.getEntryBlock()))<br>
+ DFSNumber.insert(std::make_pair(BB, ++I));<br>
+<br>
----------------<br>
</span><span class="">dberlin wrote:<br>
> Note that you can get the DFS in/out numbers from the dominator tree if that is an acceptable ordering (IE DFS on DT).<br>
><br>
</span>It seems DFS number is not always available in the dominator tree. DFS numbers are updated only if there are too many (> 32) slow queries in GenericDomTree.h:468<br>
<br>
<br></blockquote><div>YOu can guarantee it by calling updateDFSNumbers</div><div><br></div></div></div></div>