<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Dec 13, 2016 at 4:01 PM, Hal Finkel 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">hfinkel added inline comments.<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/<wbr>Scalar/GVNExpression.h:65<br>
<span class="">+      return true;<br>
+    // Compare etype for anything but load and store.<br>
</span>+    if (getExpressionType() != ET_Load &&<br>
----------------<br></blockquote><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Please explain in this comment why you're not comparing the expression types for loads and stores. </blockquote><div>Sigh.</div><div><br></div><div>If we didn't do coercion, i would say we should just clean this all up and use a single MemoryLocationExpression or something (since we are really value numbering the [memory location, memory state]), but  for coercion, we need to be able to tell if they were loads or stores.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This is also somewhat confusing because we've already compared the opcodes.</blockquote><div>Yes, though the opcodes are 0 :P.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/<wbr>Scalar/GVNExpression.h:183<br>
+  virtual bool equals(const Expression &Other) const override {<br>
+    const BasicExpression &OE = cast<BasicExpression>(Other);<br>
+    if (getOpcode() != OE.getOpcode())<br>
----------------<br>
This looks a bit odd. Shouldn't you check the opcode equality first and then cast to BasicExpression?<br>
<br></blockquote><div>Yes.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Transforms/Scalar/NewGVN.<wbr>cpp:215<br>
+  // Debugging for how many times each block and instruction got processed.<br>
+  DenseMap<const Value *, unsigned> ProcessedCount;<br>
+<br>
----------------<br>
Do we want to guard this with `#ifdef NDEBUG`?<br></blockquote><div><br></div><div>Yes.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/NewGVN.<wbr>cpp:460<br>
+// I, and see if it resulted in a simpler expression. If so, return<br>
+// that expression<br>
+// TODO: Once finished, this should not take an Instruction, we only<br>
----------------<br>
Add period after expression.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/NewGVN.<wbr>cpp:647<br>
</span>+      new (ExpressionAllocator) CallExpression(CI-><wbr>getNumOperands(), CI, HV);<br>
+  setBasicExpressionInfo(CI, E, B);<br>
+  return E;<br>
----------------<br>
We also need to add operand bundles too for calls.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D26224" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D26224</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>