<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Dec 13, 2016 at 7:27 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 a comment.<br>
<br>
A few more comments on the comments, otherwise, I'm fine with committing this and working on it in-tree. Thanks for your work on this!<br>
<br>
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/<wbr>Scalar/GVNExpression.h:66<br>
+    // Compare the expression type for anything but load and store.<br>
+    // This is needed for load coercion.<br>
<span class="">+    if (getExpressionType() != ET_Load &&<br>
----------------<br>
</span>What does load coercion mean in this context? Also, for loads and stores we set the opcode to 0 and that should be noted somewhere here.<br></blockquote><div><br></div><div>We duplicate the functionality of current GVN (though not in this patch), which "coerces" parts of existing loads to get values out of them.</div><div><br></div><div>So for example,   </div><div>given</div><div><br></div><div>load i32, %ptr</div><div>and load i16, %ptr</div><div>and</div><div>load i16, %ptr + 1 (or whatever the second half is)</div><div><br>Current GVN understands it can pull the second two values out of the first one.</div><div><br></div><div>It also understands, given</div><div><br></div><div>load i16</div><div>load i16</div><div>load i32</div><div><br></div><div><br></div><div>that it can widen the first load to i32 and eliminate the second two.</div><div><br></div><div>In practice, the types of loads/stores don't matter anyway (ignoring non-integral pointer types, which we'd probably just pass on), we can always make them correct.</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:648<br>
+                                                   const BasicBlock *B) {<br>
+  // FIXME: Add bundle operators for calls.<br>
+  CallExpression *E =<br>
----------------<br>
bundle operators -> operand bundles<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>