[PATCH] D18798: New code hoisting pass based on GVN

[Chandler Carruth via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Some high level comments:

- The code formatting seems bad. Please run clang-format at the least on the new code.
- Thanks for addressing Danny's comments about the algorithmic complexity. Keeping this away from O(n^2) algorithms is really important.
- You didn't address Danny's comment about providing motivating benchmark data showing the improvements this provides.
- 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.)


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:212-213
@@ -211,3 +211,4 @@
     FPM.add(createScalarReplAggregatesPass());
   FPM.add(createEarlyCSEPass());
+  FPM.add(createEarlyGVNPass());
   FPM.add(createLowerExpectIntrinsicPass());
----------------
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.

Either way, I strongly suspect this should be conditioned on O3...


Repository:
  rL LLVM

http://reviews.llvm.org/D18798