<div dir="ltr">To be specific (and feel free to tell me why i'm wrong, these functions are a bit hard to decipher without comments):<br><br><div>For starters, for loads/stores I don't understand why you use gatherallpaths and then check the memoryuses, instead of calculating Nearest common dominator (or postdominator, depending)(hoist point, blocks for all the uses).</div><div><br></div><div><br></div><div>Let's ignore the may-throw/etc issues for a second (which can be done with a single block test)</div><div><br></div><div>For scalar computations, hoist point safety depends on whether you can recompute the operands at that dominator, nothing else.</div><div><br></div><div>For load/stores, it's easier. For loads, if the memory state (ie the thing the clobbering memorydef defines) dominates your hoistpoint, then it becomes the same scalar question, because if you can re-make the load, you know the memory-state will be the same.</div><div>For stores, if you are sinking, you are defining the memory state in a given block, the only question is whether that memory state is killed before it the use.</div><div><br></div><div>Checking domination is not necessary, the only way it can be true is if every store produces the same VN, and it's intersection of all successors to the sink point.</div><div><br></div><div>If you are trying to *hoist* stores, it's a much simpler problem, the point you can hoist to is nearest-common-dominator(getClobberingDefinition(store)->block, blocks of all uses).</div><div> <br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 6, 2016 at 8:43 AM, 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">dberlin added inline comments.<br>
<br>
================<br>
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:278<br>
@@ +277,3 @@<br>
+  // Return true when there are users of A in one of the BBs of Paths.<br>
+  bool hasMemoryUseOnPaths(MemoryAccess *A,<br>
+                           SmallPtrSetImpl<const BasicBlock *> &Paths) {<br>
----------------<br>
I feel like this + gatherallblocks is really an up-safe/down-safe computation (depending on whether it's a load or store), and can be done much saner than you are doing it.<br>
<br>
In particular, this seems a really complicated and expensive way to compute this property, compared to how most PRE papers do it.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D19338" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19338</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>