<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, May 10, 2017 at 12:50 PM, Daniel Berlin via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.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 a comment.<br>
<span class=""><br>
In <a href="https://reviews.llvm.org/D33056#751369" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D33056#751369</a>, @davide wrote:<br>
<br>
> I think we might just be missing a check for store count (as it's done in the other place we call `getNextMemoryLeader()`).<br>
>  Please let me know what you think, and thanks for catching the mistake!<br>
><br>
>   diff --git a/lib/Transforms/Scalar/<wbr>NewGVN.cpp b/lib/Transforms/Scalar/<wbr>NewGVN.cpp<br>
>   index 3c9850b..f7182f2 100644<br>
>   --- a/lib/Transforms/Scalar/<wbr>NewGVN.cpp<br>
>   +++ b/lib/Transforms/Scalar/<wbr>NewGVN.cpp<br>
>   @@ -1381,7 +1381,7 @@ bool NewGVN::setMemoryClass(const MemoryAccess *From,<br>
>            NewClass->memory_insert(MP);<br>
>            // This may have killed the class if it had no non-memory members<br>
>            if (OldClass->getMemoryLeader() == From) {<br>
>   -          if (OldClass->memory_empty()) {<br>
>   +          if (OldClass->memory_empty() && OldClass->getStoreCount() == 0) {<br>
>                OldClass->setMemoryLeader(<wbr>nullptr);<br>
>              } else {<br>
>                OldClass->setMemoryLeader(<wbr>getNextMemoryLeader(OldClass))<wbr>;<br>
><br>
>   fluttershy@SONY-PC C:\Users\fluttershy\work\llvm<br>
><br>
<br>
<br>
</span>Oh, yes, I believe you have discovered the root cause.<br>
Sigh.<br>
I really wish it was easy to unify all this handling into one place.<br>
<br>
Can you add a definesNoMemory or something to CongruenceClass, and use it here?<br>
It should be memory_empty && storecount == 0<br>
<br>
Then we can also replace this check:<br>
<br>
  if (OldClass->getStoreCount() != 0 || !OldClass->memory_empty()) {<br>
<br>
with it<br></blockquote><div><br></div><div>In particular, this check is really !definesNoMemory</div><div><br></div><div>We could also instead add definesMemory() and use it directly here, and then the place you are fixing is !definesMemory().</div><div> </div><div>Not sure which we'd rather have.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
(This will also force us to make sure the class is in a consistent state. It would also let us add an assert that  that if (CC->definesNoMemory()), then <nothing in ->members() is a store> && and memory_empty()) in a few places.<br>
(though this is an expensive assert)<br>
<br>
<br>
<a href="https://reviews.llvm.org/D33056" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D33056</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>