<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Nov 27, 2016 at 6:14 PM, 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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
================<br>
Comment at: include/llvm/Transforms/Scalar<wbr>/GVNExpression.h:71-83<br>
+  bool operator==(const Expression &Other) const {<br>
+    if (Opcode != Other.Opcode)<br>
+      return false;<br>
+    if (Opcode == ~0U || Opcode == ~1U)<br>
+      return true;<br>
+    // Compare etype for anything but load and store.<br>
<span>+    if (getExpressionType() != ExpressionTypeLoad &&<br>
----------------<br>
</span>The behavior of this operator is quite nonintuitive. It's actually `isTrivallyCongruent()`, not is equal. However if this is the natural equality definition for this type, then it would be fine with a comment explaining that.<br>
<br></blockquote><div><br></div></span><div>This is deliberate, so yes.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: include/llvm/Transforms/Scalar<wbr>/GVNExpression.h:87-89<br>
+  virtual hash_code getHashValue() const {<br>
+    return hash_combine(EType, Opcode);<br>
+  }<br>
----------------<br>
This can return a different `hash_code` for Expressions which compare equal.<br></blockquote><div><br></div></span><div>What example are you thinking of?</div></div></div></div></blockquote><div><br></div><div>In particular, If you are thinking of loads/stores, both override getHashValue.</div><div>So, AFAIK, they get the right answers all the time.</div><div><br></div><div>It would be nice if everything was nicely commented and consistent here, however.</div><div><br></div><div>When i originally wrote it, I marked it for later cleanup, but it seems those TODO's and FIXMEs disappeared at some point :(</div><div><br></div><div>If you are thinking of the ~1U and ~0U case, these are special keys and that's just okay.<br></div><div><br></div><div>Notice also that the current GVN.cpp does the same thing with those.<br></div></div></div></div>