<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 29, 2016 at 12:14 PM, Sebastian Pop <span dir="ltr"><<a href="mailto:sebpop@gmail.com" target="_blank">sebpop@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">sebpop added inline comments.<br>
<span class=""><br>
================<br>
Comment at: llvm/lib/Transforms/Scalar/GVNHoist.cpp:761<br>
@@ +760,3 @@<br>
+            // otherwise when we try to remove OldMemAcc, MSSA gets confused and<br>
+            // removes NewMemAcc as well.<br>
+            MSSA->removeMemoryAccess(OldMemAcc);<br>
----------------<br>
</span><span class="">dberlin wrote:<br>
> What do you mean here?<br>
> That either sounds like a bug in memssa, or something :)<br>
</span>My first patch had both MemUses and MemDefs handled in the same way.<br></blockquote><div><br></div><div>What bug did it cause.</div><div>You should be able to delete them in either order, and i don't see why MSSA would break?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I had a hard time finding how to get around this, so I would rather amend the MSSA to avoid other people getting through the same problems.<br>
Let me think how to fix this in MSSA.</blockquote><div><br></div><div>What precisely happens?</div><div> </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:770<br>
@@ +769,3 @@<br>
+            // NewMemAcc before removing OldMemAcc.<br>
+            OldMemAcc->replaceAllUsesWith(NewMemAcc);<br>
+            MSSA->removeMemoryAccess(OldMemAcc);<br>
----------------<br>
</span><div><div class="h5">dberlin wrote:<br>
> Are you guaranteed that anything that conflicted with the old store conflicts with this store by your hoist safety rules?<br>
> (if not, you should only replace  inMD's and phis, not memoryuses)<br>
><br>
> it might be worth either giving an example here, or adding a testcase.<br>
><br>
> The two problems are:<br>
><br>
> 1 = MemoryDef<liveOnEntry><br>
> store<br>
> 2 = MemoryDef<1><br>
> store<br>
><br>
> later<br>
> memoryphi(2, )<br>
><br>
><br>
> If the hoist point is 1, and you simply move the store, you will now have:<br>
> 1 = MemoryDef<liveOnEntry><br>
> store<br>
> 3 = MemoryDef<1><br>
> store<br>
> 2 = MemoryDef<1><br>
> store<br>
><br>
> later<br>
> memoryphi(2, )<br>
><br>
><br>
> This is wrong.  3 is now disconnected from the graph of reaching defs.<br>
><br>
> Additionally, if the hoist point is 2, you end up with:<br>
> 1 = MemoryDef<liveOnEntry><br>
> store<br>
> 2 = MemoryDef<1><br>
> store<br>
> 3 = MemoryDef<1><br>
> store<br>
><br>
> later<br>
> memoryphi(2, )<br>
><br>
> The memoryphi is now wrong (and you disconnected 2 from the reaching graph).<br>
><br>
</div></div>I added that comment above Def to make sure it is clear that hoisting guarantees these properties:<br>
<br>
          // The definition of this ld/st will not change: ld/st hoisting is<br>
          // legal when the ld/st is not moved past its current definition.<br>
          MemoryAccess *Def = OldMemAcc->getDefiningAccess();<br>
<br>
> hoist point is 1<br>
Cannot happen: we cannot move the store "3" past store "2" as they are on the same chain (dependent.)<br>
This is an illegal case of hoisting and will be discarded.<br>
<br>
> hoist point is 2<br>
Cannot happen either as there is a use of "2" (the phi node) past which "3" cannot be moved.<br>
Again the dependence analysis will discard this hoisting as invalid.<br>
<br>
<br>
<br>
<a href="https://reviews.llvm.org/D22966" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22966</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>