<div dir="ltr">I just want to echo Chandler's comments about complexity:<div>Thank you for taking the time to do the work to not add another N^2 pass to the compiler, even if it took you a bit more time/effort :)</div><div>(I'll take another gander at the patch for code review, but you should address chandler's comments)</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr 7, 2016 at 1:26 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">chandlerc requested changes to this revision.<br>
chandlerc added a comment.<br>
This revision now requires changes to proceed.<br>
<br>
Some high level comments:<br>
<br>
- The code formatting seems bad. Please run clang-format at the least on the new code.<br>
- Thanks for addressing Danny's comments about the algorithmic complexity. Keeping this away from O(n^2) algorithms is really important.<br>
- You didn't address Danny's comment about providing motivating benchmark data showing the improvements this provides.<br>
- We also would likely need to see a reasonable analysis of the effect this has on compile time so we understand the tradeoff this is making. My suspicion is that it won't be the correct tradeoff for O2 if it is correct at any level. (See below.)<br>
<br>
<br>
================<br>
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:212-213<br>
@@ -211,3 +211,4 @@<br>
     FPM.add(createScalarReplAggregatesPass());<br>
   FPM.add(createEarlyCSEPass());<br>
+  FPM.add(createEarlyGVNPass());<br>
   FPM.add(createLowerExpectIntrinsicPass());<br>
----------------<br>
It makes very little sense IMO to do early-cse and then early-gvn... The only point of early-cse was to be a substantially lighter weight CSE than GVN. If you're going to run GVN anyways to do hoisting, just run GVN.<br>
<br>
Either way, I strongly suspect this should be conditioned on O3...<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D18798" rel="noreferrer" target="_blank">http://reviews.llvm.org/D18798</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div>