<div dir="ltr">(Not overly familiar with NewGVN, but should we also be comparing the contents of MemoryMembers in this?)</div><br><div class="gmail_quote"><div dir="ltr">On Wed, May 30, 2018 at 3:28 PM George Burgess IV via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: gbiv<br>
Date: Wed May 30 15:24:08 2018<br>
New Revision: 333601<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=333601&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=333601&view=rev</a><br>
Log:<br>
[NewGVN] Fix set comparison; reflow comment<br>
<br>
Looks like we intended to compare this->Members with Other->Members<br>
here, but ended up comparing this->Members with this->Members. Oops. :)<br>
<br>
Since CongruenceClass::Members is a SmallPtrSet anyway, we can probably<br>
skip building std::sets if we're willing to write a bit more code.<br>
<br>
This appears to be no functional change (for sufficiently lax values of<br>
"no"): this equality check was only being called inside of an assert.<br>
So, worst case, we'll catch more bugs in the form of assertion failures.<br>
<br>
Thanks to d0k for noting this!<br>
<br>
Modified:<br>
    llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp?rev=333601&r1=333600&r2=333601&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp?rev=333601&r1=333600&r2=333601&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp Wed May 30 15:24:08 2018<br>
@@ -366,9 +366,8 @@ public:<br>
   // True if this class has no memory members.<br>
   bool definesNoMemory() const { return StoreCount == 0 && memory_empty(); }<br>
<br>
-  // Return true if two congruence classes are equivalent to each other.  This<br>
-  // means<br>
-  // that every field but the ID number and the dead field are equivalent.<br>
+  // Return true if two congruence classes are equivalent to each other. This<br>
+  // means that every field but the ID number and the dead field are equivalent.<br>
   bool isEquivalentTo(const CongruenceClass *Other) const {<br>
     if (!Other)<br>
       return false;<br>
@@ -383,10 +382,12 @@ public:<br>
       if (!DefiningExpr || !Other->DefiningExpr ||<br>
           *DefiningExpr != *Other->DefiningExpr)<br>
         return false;<br>
-    // We need some ordered set<br>
-    std::set<Value *> AMembers(Members.begin(), Members.end());<br>
-    std::set<Value *> BMembers(Members.begin(), Members.end());<br>
-    return AMembers == BMembers;<br>
+<br>
+    if (Members.size() != Other->Members.size())<br>
+      return false;<br>
+<br>
+    return all_of(Members,<br>
+                  [&](const Value *V) { return Other->Members.count(V); });<br>
   }<br>
<br>
 private:<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>